Check whether element is in document before calling jQuery.fn.offset()

[kevin-j-morse]

This pull request includes a

• Bug fix
• New feature
• Translation

The following changes were made

As of jQuery 3.0, jQuery.fn.offset() requires an element connected to a document. In earlier versions of jQuery, the .offset() method would return a value of { top: 0, left: 0 } for some cases of invalid input. jQuery 3.0 throws errors in some of these cases.

This PR checks whether the element is within the document. If not, it sets the offset to { top: 0, left: 0 } which is what was happening before. I believe this covers all valid scenarios but perhaps someone else can comment?

See the jQuery migrate documentation for more info. https://github.com/jquery/jquery-migrate/blob/master/warnings.md#jqmigrate-jqueryfnoffset-requires-an-element-connected-to-a-document

[kevin-brown]

I know this might be tough, since our positioning logic is not the easiest to read and test, but would it be possible for you to create a test for this?

It wouldn't be a whole lot different from

select2/tests/dropdown/positioning-tests.js

Lines 61 to 119 in 2fce8ae

	test('dropdown is positioned down with static margins', function (assert) {
	var $ = require('jquery');
	var $select = $('<select></select>');
	var $parent = $('<div></div>');
	$parent.css({
	position: 'static',
	marginTop: '5px',
	marginLeft: '10px'
	});
	var $container = $('<span>test</span>');
	var container = new MockContainer();
	$('#qunit-fixture').empty();
	$parent.appendTo($('#qunit-fixture'));
	$container.appendTo($parent);
	var Utils = require('select2/utils');
	var Options = require('select2/options');
	var Dropdown = require('select2/dropdown');
	var AttachBody = require('select2/dropdown/attachBody');
	var DropdownAdapter = Utils.Decorate(Dropdown, AttachBody);
	var dropdown = new DropdownAdapter($select, new Options({
	dropdownParent: $parent
	}));
	var $dropdown = dropdown.render();
	assert.equal(
	$dropdown[0].style.top,
	0,
	'The drodpown should not have any offset before it is displayed'
	);
	dropdown.bind(container, $container);
	dropdown.position($dropdown, $container);
	dropdown._showDropdown();
	assert.ok(
	dropdown.$dropdown.hasClass('select2-dropdown--below'),
	'The dropdown should be forced down'
	);
	assert.equal(
	$dropdown.css('top').replace(/\D+/, ''),
	$container.outerHeight() + 5,
	'The offset should be 5px at the top'
	);
	assert.equal(
	$dropdown.css('left'),
	'10px',
	'The offset should be 10px on the left'
	);
	});

Except you don't need to play around with the CSS and you don't need to attach it to the QUnit fixture (since that would add it into the document).

[kevin-j-morse]

I will see what I can do.

…

On Tue., Jul. 23, 2019, 05:18 Kevin Brown, ***@***.***> wrote: I know this might be tough, since our positioning logic is not the easiest to read and test, but would it be possible for you to create a test for this? It wouldn't be a whole lot different from https://github.com/select2/select2/blob/2fce8ae6c4937b88c332ad3e30e135cf24504cf1/tests/dropdown/positioning-tests.js#L61-L119 Except you don't need to play around with the CSS and you don't need to attach it to the QUnit fixture (since that would add it into the document). — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#5584?email_source=notifications&email_token=ABBVGEKHKIN27SPWTQNRPTTQA3ZIXA5CNFSM4IGA6KZKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD2S5M3A#issuecomment-514184812>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABBVGEILKJCNXRVJJZZX3ADQA3ZIXANCNFSM4IGA6KZA> .

[kevin-j-morse]

@kevin-brown should this test live within select2/tests/dropdown/positioning-tests.js

[kevin-brown]

should this test live within select2/tests/dropdown/positioning-tests.js

That sounds like a good place for it, yes.

[kevin-brown]

In the interest of getting 4.0.9 out the door without procrastinating even further, I'm moving this PR to land in 4.0.10.

[kevin-j-morse]

Sorry. I have been swamped with work lately. I will try to get to this soon.

…

On Wed, Aug 21, 2019 at 5:31 PM Kevin Brown ***@***.***> wrote: In the interest of getting 4.0.9 out the door without procrastinating even further, I'm moving this PR to land in 4.0.10. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#5584?email_source=notifications&email_token=ABBVGEJYGU5ZILH7IT544WTQFXM5LA5CNFSM4IGA6KZKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD43QHMQ#issuecomment-523699122>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABBVGEM6ORWSBYPLMKLLFYDQFXM5LANCNFSM4IGA6KZA> .