From c56571ce3400e86b5cf9b4007bc1fe2bee68adf5 Mon Sep 17 00:00:00 2001 From: Mike Countis Date: Tue, 21 Jan 2020 12:16:11 -0800 Subject: [PATCH 1/2] Prevent a disabled element from opening via keydown/keypress events Gist: return early from the Select2#open method if the object is disabled. Add helper #isEnabled and #isDisabled methods to Select2 and selector classes. Add test coverage for the original, expected behavior and new preventative behavior. --- src/js/select2/core.js | 29 +++- src/js/select2/selection/allowClear.js | 4 +- src/js/select2/selection/base.js | 21 +++ src/js/select2/selection/multiple.js | 2 +- tests/selection/openOnKeyDown-tests.js | 201 +++++++++++++++++++++++++ tests/unit-jq1.html | 1 + tests/unit-jq2.html | 1 + tests/unit-jq3.html | 1 + 8 files changed, 255 insertions(+), 5 deletions(-) create mode 100644 tests/selection/openOnKeyDown-tests.js diff --git a/src/js/select2/core.js b/src/js/select2/core.js index 92084143ac..307b824ac6 100644 --- a/src/js/select2/core.js +++ b/src/js/select2/core.js @@ -365,7 +365,7 @@ define([ Select2.prototype._syncAttributes = function () { this.options.set('disabled', this.$element.prop('disabled')); - if (this.options.get('disabled')) { + if (this.isDisabled()) { if (this.isOpen()) { this.close(); } @@ -455,7 +455,7 @@ define([ }; Select2.prototype.toggleDropdown = function () { - if (this.options.get('disabled')) { + if (this.isDisabled()) { return; } @@ -471,6 +471,10 @@ define([ return; } + if (this.isDisabled()) { + return; + } + this.trigger('query', {}); }; @@ -482,6 +486,27 @@ define([ this.trigger('close', {}); }; + /** + * Helper method to abstract the "enabled" (not "disabled") state of this + * object. + * + * @return {true} if the instance is not disabled. + * @return {false} if the instance is disabled. + */ + Select2.prototype.isEnabled = function () { + return !this.isDisabled(); + }; + + /** + * Helper method to abstract the "disabled" state of this object. + * + * @return {true} if the disabled option is true. + * @return {false} if the disabled option is false. + */ + Select2.prototype.isDisabled = function () { + return this.options.get('disabled'); + }; + Select2.prototype.isOpen = function () { return this.$container.hasClass('select2-container--open'); }; diff --git a/src/js/select2/selection/allowClear.js b/src/js/select2/selection/allowClear.js index 0de5b9bbbd..e112a43287 100644 --- a/src/js/select2/selection/allowClear.js +++ b/src/js/select2/selection/allowClear.js @@ -31,7 +31,7 @@ define([ AllowClear.prototype._handleClear = function (_, evt) { // Ignore the event if it is disabled - if (this.options.get('disabled')) { + if (this.isDisabled()) { return; } @@ -97,7 +97,7 @@ define([ return; } - var removeAll = this.options.get('translations').get('removeAllItems'); + var removeAll = this.options.get('translations').get('removeAllItems'); var $remove = $( '' + diff --git a/src/js/select2/selection/base.js b/src/js/select2/selection/base.js index f3999b8316..ed9c50d327 100644 --- a/src/js/select2/selection/base.js +++ b/src/js/select2/selection/base.js @@ -153,5 +153,26 @@ define([ throw new Error('The `update` method must be defined in child classes.'); }; + /** + * Helper method to abstract the "enabled" (not "disabled") state of this + * object. + * + * @return {true} if the instance is not disabled. + * @return {false} if the instance is disabled. + */ + BaseSelection.prototype.isEnabled = function () { + return !this.isDisabled(); + }; + + /** + * Helper method to abstract the "disabled" state of this object. + * + * @return {true} if the disabled option is true. + * @return {false} if the disabled option is false. + */ + BaseSelection.prototype.isDisabled = function () { + return this.options.get('disabled'); + }; + return BaseSelection; }); diff --git a/src/js/select2/selection/multiple.js b/src/js/select2/selection/multiple.js index 17afa4e40e..cfd6029c9a 100644 --- a/src/js/select2/selection/multiple.js +++ b/src/js/select2/selection/multiple.js @@ -37,7 +37,7 @@ define([ '.select2-selection__choice__remove', function (evt) { // Ignore the event if it is disabled - if (self.options.get('disabled')) { + if (self.isDisabled()) { return; } diff --git a/tests/selection/openOnKeyDown-tests.js b/tests/selection/openOnKeyDown-tests.js new file mode 100644 index 0000000000..fdf454f579 --- /dev/null +++ b/tests/selection/openOnKeyDown-tests.js @@ -0,0 +1,201 @@ +module('Selection containers - Open On Key Down'); + +var KEYS = require('select2/keys'); +var $ = require('jquery'); + +/** + * Fetching method to return the fixtures container to append new select + * elements. Rendering/opening a select2 element that is not in the DOM will + * result in the select2 container element being appened to the end of the + * resulting in those elements showing on the page rather than being + * hidden or rendered far off the page. + * + * @return {jQuery} a jQuery selection to be appended to. + */ +function $fixtures () { + return $(document.body).find('#qunit-fixture'); +} + +/** + * Build a keydown event with the given key code and extra options. + * + * @param {Number} keyCode the keyboard code to be used for the 'which' + * attribute of the keydown event. + * @param {Object} eventProps extra properties to build the keydown event. + * + * @return {jQuery.Event} a 'keydown' type event. + */ +function buildKeyDownEvent (keyCode, eventProps) { + return $.Event('keydown', $.extend({}, { which: keyCode }, eventProps)); +} + +/** + * Wrapper function providing a select2 element with a given enabled/disabled + * state that will get a given keydown event triggered on it. Provide an + * assertion callback function to test the results of the triggered event. + * + * @param {Boolean} isEnabled the enabled state of the desired select2 + * element. + * @param {String} testName name for the test. + * @param {Number} keyCode used to set the 'which' attribute of the + * keydown event. + * @param {Object} eventProps attributes to be used to build the keydown + * event. + * @param {Function} fn assertion callback to perform checks on the + * result of triggering the event, receives the + * 'assert' variable for the test and the select2 + * instance behind the built ' + + '' + + '' + + '' + ); + $fixtures().append($element); + $element.select2({ disabled: !isEnabled }); + + var select2 = $element.data('select2'); + var $selection = select2.$selection; + + assert.notOk(select2.isOpen(), 'The instance should not be open'); + assert.equal(select2.isEnabled(), isEnabled); + + var event = buildKeyDownEvent(keyCode, eventProps); + assert.ok(event.which, 'The event\'s key code (.which) should be set'); + + $selection.trigger(event); + + fn(assert, select2); + }); +} + +/** + * Test the given keydown event on an enabled element. See #testAbled for + * params. + */ +function testEnabled (testName, keyCode, eventProps, fn) { + testAbled(true, testName, keyCode, eventProps, fn); +} + +/** + * Test the given keydown event on a disabled element. See #testAbled for + * params. + */ +function testDisabled (testName, keyCode, eventProps, fn) { + testAbled(false, testName, keyCode, eventProps, fn); +} + +/** + * Assertion function used by the above test* wrappers. Asserts that the given + * select2 instance is open. + * + * @param {Assert} assert + * @param {Select2} select + * @return {null} + */ +function assertOpened (assert, select2) { + assert.ok(select2.isOpen(), 'The element should be open'); +} + +/** + * Assertion function used by the above test* wrappers. Asserts that the given + * select2 instance is not open. + * + * @param {Assert} assert + * @param {Select2} select + * @return {null} + */ +function assertNotOpened (assert, select2) { + assert.notOk(select2.isOpen(), 'The element should not be open'); +} + +/** + * ENTER, SPACE, and ALT+DOWN should all open an enabled select2 element. + */ +testEnabled( + 'enabled element will open on ENTER', + KEYS.ENTER, {}, + assertOpened +); +testEnabled( + 'enabled element will open on SPACE', + KEYS.SPACE, {}, + assertOpened +); +testEnabled( + 'enabled element will open on ALT+DOWN', + KEYS.DOWN, { altKey: true }, + assertOpened +); + +/** + * Some other keys triggered on an enabled select2 element should not open it. + */ +testEnabled( + 'enabled element will not open on UP', + KEYS.UP, {}, + assertNotOpened +); +testEnabled( + 'enabled element will not open on DOWN', + KEYS.UP, {}, + assertNotOpened +); +testEnabled( + 'enabled element will not open on LEFT', + KEYS.UP, {}, + assertNotOpened +); +testEnabled( + 'enabled element will not open on RIGHT', + KEYS.UP, {}, + assertNotOpened +); + +/* + * The keys that will open an enabled select2 element should not open a disabled + * one. + */ +testDisabled( + 'disabled element will not open on ENTER', + KEYS.ENTER, {}, + assertNotOpened +); +testDisabled( + 'disabled element will not open on SPACE', + KEYS.SPACE, {}, + assertNotOpened +); +testDisabled( + 'disabled element will not open on ALT+DOWN', + KEYS.DOWN, { altKey: true }, + assertNotOpened +); + +/** + * Other keys should continue to not open a disabled select2 element. + */ +testDisabled( + 'disabled element will not open on UP', + KEYS.UP, {}, + assertNotOpened +); +testDisabled( + 'disabled element will not open on DOWN', + KEYS.UP, {}, + assertNotOpened +); +testDisabled( + 'disabled element will not open on LEFT', + KEYS.UP, {}, + assertNotOpened +); +testDisabled( + 'disabled element will not open on RIGHT', + KEYS.UP, {}, + assertNotOpened +); diff --git a/tests/unit-jq1.html b/tests/unit-jq1.html index b5ec346126..f1dac344f5 100644 --- a/tests/unit-jq1.html +++ b/tests/unit-jq1.html @@ -97,6 +97,7 @@ + diff --git a/tests/unit-jq2.html b/tests/unit-jq2.html index 7eca50575b..9d4a99b15e 100644 --- a/tests/unit-jq2.html +++ b/tests/unit-jq2.html @@ -97,6 +97,7 @@ + diff --git a/tests/unit-jq3.html b/tests/unit-jq3.html index a34f771c93..9e062678a6 100644 --- a/tests/unit-jq3.html +++ b/tests/unit-jq3.html @@ -97,6 +97,7 @@ + From 09b233bf28985d5401d800ed1eed3ed382fd851f Mon Sep 17 00:00:00 2001 From: Kevin Brown Date: Mon, 27 Jan 2020 21:16:13 -0500 Subject: [PATCH 2/2] Remove single-use method from tests --- tests/selection/openOnKeyDown-tests.js | 15 +-------------- 1 file changed, 1 insertion(+), 14 deletions(-) diff --git a/tests/selection/openOnKeyDown-tests.js b/tests/selection/openOnKeyDown-tests.js index fdf454f579..0da838e2c4 100644 --- a/tests/selection/openOnKeyDown-tests.js +++ b/tests/selection/openOnKeyDown-tests.js @@ -3,19 +3,6 @@ module('Selection containers - Open On Key Down'); var KEYS = require('select2/keys'); var $ = require('jquery'); -/** - * Fetching method to return the fixtures container to append new select - * elements. Rendering/opening a select2 element that is not in the DOM will - * result in the select2 container element being appened to the end of the - * resulting in those elements showing on the page rather than being - * hidden or rendered far off the page. - * - * @return {jQuery} a jQuery selection to be appended to. - */ -function $fixtures () { - return $(document.body).find('#qunit-fixture'); -} - /** * Build a keydown event with the given key code and extra options. * @@ -55,7 +42,7 @@ function testAbled(isEnabled, testName, keyCode, eventProps, fn) { '' + '' ); - $fixtures().append($element); + $('#qunit-fixture').append($element); $element.select2({ disabled: !isEnabled }); var select2 = $element.data('select2');