From 936abfe45bd11fe40ef8892688fdc72ebff62846 Mon Sep 17 00:00:00 2001 From: Kevin Brown Date: Sun, 4 Aug 2019 21:39:26 -0400 Subject: [PATCH 1/2] allowClear no longer shifts selections to a new line This fixes an issue that we have had with the "x" icon used by the `allowClear` option where selections that just barely interacted with the position of the "x" icon would be pushed to a new line that was separate from the normal second line of selections. This case was pretty rare, because you only had a ~9px area where the interaction could occur. The issue was cuased by the "x" icon being sized for the height of the text in the selection choices, which should be the same as how the selection choices themselves are sized. Unfortunately this did not take into account the fact that the selection choices are given a 1px border which increases their size by 2px, which is what lead to the odd behaviour. This behaviour could not be replicated without the 1px border because the height would then line up correctly. The issue can be fixed by adding a 2px margin to the bottom of the "x" icon, which would force overlapping selections on to the correct second line of selections. This was the method that many users have been using to correct this issue, but was not the method we chose to use. A 1px padding has been added to the "x" icon instead, which should expand the touch area of the "x" by a little while also increasing the height of the "x" by enough to prevent the overlapping. Fixes #4470 --- src/scss/theme/default/_multiple.scss | 6 ++++ tests/selection/allowClear-tests.js | 48 +++++++++++++++++++++++++++ 2 files changed, 54 insertions(+) diff --git a/src/scss/theme/default/_multiple.scss b/src/scss/theme/default/_multiple.scss index e6332ac7b2..55852002ec 100644 --- a/src/scss/theme/default/_multiple.scss +++ b/src/scss/theme/default/_multiple.scss @@ -22,6 +22,12 @@ font-weight: bold; margin-top: 5px; margin-right: 10px; + + // This padding is to account for the bottom border for the first + // selection row and the top border of the second selection row. + // Without it, selections on the first row may be offset incorrectly + // and appear in their own row instead of going to the second row + padding: 1px; } .select2-selection__choice { diff --git a/tests/selection/allowClear-tests.js b/tests/selection/allowClear-tests.js index a7ca726daf..85d9d62288 100644 --- a/tests/selection/allowClear-tests.js +++ b/tests/selection/allowClear-tests.js @@ -4,6 +4,7 @@ var Placeholder = require('select2/selection/placeholder'); var AllowClear = require('select2/selection/allowClear'); var SingleSelection = require('select2/selection/single'); +var MultipleSelection = require('select2/selection/multiple'); var $ = require('jquery'); var Options = require('select2/options'); @@ -328,3 +329,50 @@ test('clear does not work when disabled', function (assert) { 'The placeholder should not have been set' ); }); + +test('clear button doesnt visually break selected options', function (assert) { + var $element = $(''); + + var Selection = Utils.Decorate( + Utils.Decorate(MultipleSelection, Placeholder), + AllowClear + ); + + var selection = new Selection( + $element, + allowClearOptions + ); + var container = new MockContainer(); + + var $container = $( + '' + ); + $('#qunit-fixture').append($container); + + var $selection = selection.render(); + $container.append($selection); + $container.css('width', '100px'); + + selection.bind(container, $container); + + selection.update([{ + id: '' + }]); + + selection.update([ + { + id: '1', + text: '1' + }, + { + id: '2', + text: '2' + } + ]); + + assert.equal( + $container.height(), + 56, + 'There should be two full lines of selections' + ); +}); From 5b6a77f41489bca727e60ba43673cb1f8acb48b2 Mon Sep 17 00:00:00 2001 From: Kevin Brown Date: Sun, 4 Aug 2019 21:57:32 -0400 Subject: [PATCH 2/2] Remove hard-coded height in tests Because tests are executed on different browsers, and because each browser sets their own line height, we cannot depend on the height of the default Select2 being consistent across browsers. As a result, we must write our tests to calcualte the expected height based on known data. In the case of this test, we can calculate ahead of time what two rows of selections is supposed to look like, instead of the edge case that we can otherwise encounter. --- tests/selection/allowClear-tests.js | 26 ++++++++++++++++++++++++-- 1 file changed, 24 insertions(+), 2 deletions(-) diff --git a/tests/selection/allowClear-tests.js b/tests/selection/allowClear-tests.js index 85d9d62288..273f80fdac 100644 --- a/tests/selection/allowClear-tests.js +++ b/tests/selection/allowClear-tests.js @@ -356,9 +356,25 @@ test('clear button doesnt visually break selected options', function (assert) { selection.bind(container, $container); selection.update([{ - id: '' + id: '1', + text: '1' }]); + var singleHeight = $container.height(); + + selection.update([ + { + id: '10', + text: '10' + }, + { + id: '20', + text: '20' + } + ]); + + var doubleHeight = $container.height(); + selection.update([ { id: '1', @@ -370,9 +386,15 @@ test('clear button doesnt visually break selected options', function (assert) { } ]); + assert.notEqual( + singleHeight, + doubleHeight, + 'The height of the two different rows should be different' + ); + assert.equal( $container.height(), - 56, + doubleHeight, 'There should be two full lines of selections' ); });