From efbfd14414bd749143fbac386ff163cf5c5be8c8 Mon Sep 17 00:00:00 2001 From: Kevin Brown Date: Sun, 28 Jul 2019 00:36:51 -0400 Subject: [PATCH] Remove selection title attribute if text is empty (#5589) This fixes a bug that was introduced in Select2 4.0.0 and only partially fixed in Select2 4.0.6-rc.0 where the `title` attribute that is set on the selection container (or individual selections, for a multiple select) is not cleared when the text/title of the option is not set. In most cases, users no longer see this issue because the `text` property of most data objects is set, so the `title` attribute will always be cleared correctly. There was a bug for cases where the `text` property was not set, or where the `text` property was set to an empty string, that resulted in the `title` attribute persisting with the incorrect value. We have fixed this issue by always removing the `title` attribute from the selection (or not adding it in the first place, for a multiple select) when the `text` and `title` properties of the data object are empty or not present. This also adds in a series of tests to ensure the `title` attribute is set properly in a variety of cases, building upon the ones that already existed. Fixes #3895 --- src/js/select2/selection/multiple.js | 7 +- src/js/select2/selection/single.js | 9 ++- tests/selection/multiple-tests.js | 113 +++++++++++++++++++++++++- tests/selection/single-tests.js | 116 ++++++++++++++++++++++++++- 4 files changed, 236 insertions(+), 9 deletions(-) diff --git a/src/js/select2/selection/multiple.js b/src/js/select2/selection/multiple.js index 6f10678817..17afa4e40e 100644 --- a/src/js/select2/selection/multiple.js +++ b/src/js/select2/selection/multiple.js @@ -95,7 +95,12 @@ define([ var formatted = this.display(selection, $selection); $selection.append(formatted); - $selection.attr('title', selection.title || selection.text); + + var title = selection.title || selection.text; + + if (title) { + $selection.attr('title', title); + } Utils.StoreData($selection[0], 'data', selection); diff --git a/src/js/select2/selection/single.js b/src/js/select2/selection/single.js index f34c3a1270..5db23e9b1f 100644 --- a/src/js/select2/selection/single.js +++ b/src/js/select2/selection/single.js @@ -93,7 +93,14 @@ define([ var formatted = this.display(selection, $rendered); $rendered.empty().append(formatted); - $rendered.attr('title', selection.title || selection.text); + + var title = selection.title || selection.text; + + if (title) { + $rendered.attr('title', title); + } else { + $rendered.removeAttr('title'); + } }; return SingleSelection; diff --git a/tests/selection/multiple-tests.js b/tests/selection/multiple-tests.js index 5e996ea6bb..ddc7c9c9e0 100644 --- a/tests/selection/multiple-tests.js +++ b/tests/selection/multiple-tests.js @@ -72,12 +72,119 @@ test('empty update clears the selection', function (assert) { var $rendered = $selection.find('.select2-selection__rendered'); $rendered.text('testing'); - $rendered.attr('title', 'testing'); selection.update([]); - assert.equal($rendered.text(), ''); - assert.equal($rendered.attr('title'), undefined); + assert.equal( + $rendered.text(), + '', + 'There should have been nothing rendered' + ); +}); + +test('empty update clears the selection title', function (assert) { + var selection = new MultipleSelection( + $('#qunit-fixture .multiple'), + options + ); + + var $selection = selection.render(); + + selection.update([]); + + var $rendered = $selection.find('.select2-selection__rendered li'); + + assert.equal( + $rendered.attr('title'), + undefined, + 'The title should be removed if nothing is rendered' + ); +}); + +test('update sets the title to the data text', function (assert) { + var selection = new MultipleSelection( + $('#qunit-fixture .multiple'), + options + ); + + var $selection = selection.render(); + + selection.update([{ + text: 'test' + }]); + + var $rendered = $selection.find('.select2-selection__rendered li'); + + assert.equal( + $rendered.attr('title'), + 'test', + 'The title should have been set to the text' + ); +}); + +test('update sets the title to the data title', function (assert) { + var selection = new MultipleSelection( + $('#qunit-fixture .multiple'), + options + ); + + var $selection = selection.render(); + + selection.update([{ + text: 'test', + title: 'correct' + }]); + + var $rendered = $selection.find('.select2-selection__rendered li'); + + assert.equal( + $rendered.attr('title'), + 'correct', + 'The title should have taken precedence over the text' + ); +}); + +test('update should clear title for placeholder options', function (assert) { + var selection = new MultipleSelection( + $('#qunit-fixture .multiple'), + options + ); + + var $selection = selection.render(); + + selection.update([{ + id: '', + text: '' + }]); + + var $rendered = $selection.find('.select2-selection__rendered li'); + + assert.equal( + $rendered.attr('title'), + undefined, + 'The title should be removed if a placeholder is rendered' + ); +}); + +test('update should clear title for options without text', function (assert) { + var selection = new MultipleSelection( + $('#qunit-fixture .multiple'), + options + ); + + var $selection = selection.render(); + + selection.update([{ + id: '' + }]); + + var $rendered = $selection.find('.select2-selection__rendered li'); + + assert.equal( + $rendered.attr('title'), + undefined, + 'The title should be removed if there is no text or title property' + ); }); test('escapeMarkup is being used', function (assert) { diff --git a/tests/selection/single-tests.js b/tests/selection/single-tests.js index 9ab163672a..99b37e66f1 100644 --- a/tests/selection/single-tests.js +++ b/tests/selection/single-tests.js @@ -50,7 +50,7 @@ test('templateSelection can addClass', function (assert) { ); var $container = selection.selectionContainer(); - + var out = selection.display({ text: 'test' }, $container); @@ -58,7 +58,7 @@ test('templateSelection can addClass', function (assert) { assert.ok(called); assert.equal(out, 'test'); - + assert.ok($container.hasClass('testclass')); }); @@ -72,12 +72,34 @@ test('empty update clears the selection', function (assert) { var $rendered = $selection.find('.select2-selection__rendered'); $rendered.text('testing'); + + selection.update([]); + + assert.equal( + $rendered.text(), + '', + 'There should have been nothing rendered' + ); +}); + +test('empty update clears the selection title', function (assert) { + var selection = new SingleSelection( + $('#qunit-fixture .single'), + options + ); + + var $selection = selection.render(); + var $rendered = $selection.find('.select2-selection__rendered'); + $rendered.attr('title', 'testing'); selection.update([]); - assert.equal($rendered.text(), ''); - assert.equal($rendered.attr('title'), undefined); + assert.equal( + $rendered.attr('title'), + undefined, + 'The title should be removed if nothing is rendered' + ); }); test('update renders the data text', function (assert) { @@ -96,6 +118,92 @@ test('update renders the data text', function (assert) { assert.equal($rendered.text(), 'test'); }); +test('update sets the title to the data text', function (assert) { + var selection = new SingleSelection( + $('#qunit-fixture .single'), + options + ); + + var $selection = selection.render(); + var $rendered = $selection.find('.select2-selection__rendered'); + + selection.update([{ + text: 'test' + }]); + + assert.equal( + $rendered.attr('title'), + 'test', + 'The title should have been set to the text' + ); +}); + +test('update sets the title to the data title', function (assert) { + var selection = new SingleSelection( + $('#qunit-fixture .single'), + options + ); + + var $selection = selection.render(); + var $rendered = $selection.find('.select2-selection__rendered'); + + selection.update([{ + text: 'test', + title: 'correct' + }]); + + assert.equal( + $rendered.attr('title'), + 'correct', + 'The title should have taken precedence over the text' + ); +}); + +test('update should clear title for placeholder options', function (assert) { + var selection = new SingleSelection( + $('#qunit-fixture .single'), + options + ); + + var $selection = selection.render(); + var $rendered = $selection.find('.select2-selection__rendered'); + + $rendered.attr('title', 'testing'); + + selection.update([{ + id: '', + text: '' + }]); + + assert.equal( + $rendered.attr('title'), + undefined, + 'The title should be removed if a placeholder is rendered' + ); +}); + +test('update should clear title for options without text', function (assert) { + var selection = new SingleSelection( + $('#qunit-fixture .single'), + options + ); + + var $selection = selection.render(); + var $rendered = $selection.find('.select2-selection__rendered'); + + $rendered.attr('title', 'testing'); + + selection.update([{ + id: '' + }]); + + assert.equal( + $rendered.attr('title'), + undefined, + 'The title should be removed if there is no text or title property' + ); +}); + test('escapeMarkup is being used', function (assert) { var selection = new SingleSelection( $('#qunit-fixture .single'),