Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

feat(jqLite): return [] for .val() on <select multiple> with no selection #15104

Merged
merged 4 commits into from
Sep 8, 2016
Merged

feat(jqLite): return [] for .val() on <select multiple> with no selection #15104

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6341f42
refactor(jqLite): run more tests on jQuery 2.2, add version detection…
mgol Sep 7, 2016
dbb8483
refactor(matchers): add the toEqualOneOf matcher
mgol Sep 7, 2016
121f649
refactor(jqLite): use the toEqualOneOf matcher in jqLite tests
mgol Sep 7, 2016
d882fde
feat(jqLite): return [] for .val() on <select multiple> with no selec…
mgol Sep 7, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/jqLite.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -699,7 +699,7 @@ forEach({
result.push(option.value || option.text);
}
});
return result.length === 0 ? null : result;
return result;
}
return element.value;
}
Expand Down
21 changes: 17 additions & 4 deletions test/helpers/matchers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,12 @@ beforeEach(function() {
};
}

function DOMTester(a, b) {
if (a && b && a.nodeType > 0 && b.nodeType > 0) {
return a === b;
}
}

function isNgElementHidden(element) {
// we need to check element.getAttribute for SVG nodes
var hidden = true;
Expand Down Expand Up @@ -111,12 +117,19 @@ beforeEach(function() {
};
}
};
},

function DOMTester(a, b) {
if (a && b && a.nodeType > 0 && b.nodeType > 0) {
return a === b;
toEqualOneOf: function(util) {
return {
compare: function(actual) {
var expectedArgs = Array.prototype.slice.call(arguments, 1);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can sliceArgs(arguments, 1) be used here instead (for readability)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's kind of weird to use internal Angular functions in matchers. IMO tests shouldn't depend on the code they're testing. Also, all other relevant matchers in this file use this technique so changing it just here would be inconsistent.

return {
pass: expectedArgs.some(function(expected) {
return util.equals(actual, expected, [DOMTester]);
})
};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't this need an error message as well?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it constructs a default error message from the matcher name... It prints something like "expected sth to equal one of null, []".

}
}
};
},

toEqualData: function() {
Expand Down
180 changes: 98 additions & 82 deletions test/jqLiteSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,19 @@
describe('jqLite', function() {
var scope, a, b, c, document;

// Checks if jQuery 2.1 is used.
function isJQuery21() {
if (_jqLiteMode) return false;
var jQueryVersionParts = _jQuery.fn.jquery.split('.');
return jQueryVersionParts[0] + '.' + jQueryVersionParts[1] === '2.1';
}

// Checks if jQuery 2.x is used.
function isJQuery2x() {
if (_jqLiteMode) return false;
var jQueryVersionParts = _jQuery.fn.jquery.split('.');
return jQueryVersionParts[0] === '2';
}

beforeEach(module(provideLog));

Expand Down Expand Up @@ -83,30 +96,27 @@ describe('jqLite', function() {
});


// This is not working correctly in jQuery prior to v3.0.
// This is not working correctly in jQuery prior to v2.2.
// See https://github.com/jquery/jquery/issues/1987 for details.
it('should properly handle dash-delimited node names', function() {
var jQueryVersion = window.jQuery && window.jQuery.fn.jquery.split('.')[0];
var jQuery3xOrNewer = jQueryVersion && (Number(jQueryVersion) >= 3);
if (isJQuery21()) return;

if (_jqLiteMode || jQuery3xOrNewer) {
var nodeNames = 'thead tbody tfoot colgroup caption tr th td div kung'.split(' ');
var nodeNamesTested = 0;
var nodes, customNodeName;
var nodeNames = 'thead tbody tfoot colgroup caption tr th td div kung'.split(' ');
var nodeNamesTested = 0;
var nodes, customNodeName;

forEach(nodeNames, function(nodeName) {
var customNodeName = nodeName + '-foo';
var nodes = jqLite('<' + customNodeName + '>Hello, world !</' + customNodeName + '>');
forEach(nodeNames, function(nodeName) {
var customNodeName = nodeName + '-foo';
var nodes = jqLite('<' + customNodeName + '>Hello, world !</' + customNodeName + '>');

expect(nodes.length).toBe(1);
expect(nodeName_(nodes)).toBe(customNodeName);
expect(nodes.html()).toBe('Hello, world !');
expect(nodes.length).toBe(1);
expect(nodeName_(nodes)).toBe(customNodeName);
expect(nodes.html()).toBe('Hello, world !');

nodeNamesTested++;
});
nodeNamesTested++;
});

expect(nodeNamesTested).toBe(10);
}
expect(nodeNamesTested).toBe(10);
});


Expand Down Expand Up @@ -712,11 +722,9 @@ describe('jqLite', function() {
describe('class', function() {

it('should properly do with SVG elements', function() {
// This is not working correctly in jQuery prior to v3.0.
// This is not working correctly in jQuery prior to v2.2.
// See https://github.com/jquery/jquery/issues/2199 for details.
var jQueryVersion = window.jQuery && window.jQuery.fn.jquery.split('.')[0];
var jQuery3xOrNewer = jQueryVersion && (Number(jQueryVersion) >= 3);
if (!_jqLiteMode && !jQuery3xOrNewer) return;
if (isJQuery21()) return;

var svg = jqLite('<svg><rect></rect></svg>');
var rect = svg.children();
Expand Down Expand Up @@ -1018,24 +1026,38 @@ describe('jqLite', function() {
'<option>test 2</option>' +
'</select>').val()).toEqual(['test 1']);

// In jQuery >= 3.0 .val() on select[multiple] with no selected options returns an
// empty array, not null.
// See https://github.com/jquery/jquery/issues/2562 for more details.
// jqLite will align with jQuery 3.0 behavior in Angular 1.6.
var val;
var jQueryVersion = window.jQuery && window.jQuery.fn.jquery.split('.')[0];
var jQuery3xOrNewer = jQueryVersion && (Number(jQueryVersion) >= 3);
if (!_jqLiteMode && jQuery3xOrNewer) {
val = [];
} else {
val = null;
}

// In jQuery < 3.0 .val() on select[multiple] with no selected options returns an
// null instead of an empty array.
expect(jqLite(
'<select multiple>' +
'<option>test 1</option>' +
'<option>test 2</option>' +
'</select>').val()).toEqual(val);
'</select>').val()).toEqualOneOf(null, []);
});

it('should get an empty array from a multi select if no elements are chosen', function() {
// In jQuery < 3.0 .val() on select[multiple] with no selected options returns an
// null instead of an empty array.
// See https://github.com/jquery/jquery/issues/2562 for more details.
if (isJQuery2x()) return;

expect(jqLite(
'<select multiple>' +
'<optgroup>' +
'<option>test 1</option>' +
'<option>test 2</option>' +
'</optgroup>' +
'<option>test 3</option>' +
'</select>').val()).toEqual([]);

expect(jqLite(
'<select multiple>' +
'<optgroup disabled>' +
'<option>test 1</option>' +
'<option>test 2</option>' +
'</optgroup>' +
'<option disabled>test 3</option>' +
'</select>').val()).toEqual([]);
});
});

Expand Down Expand Up @@ -1070,7 +1092,8 @@ describe('jqLite', function() {

describe('on', function() {
it('should bind to window on hashchange', function() {
if (jqLite.fn) return; // don't run in jQuery
if (!_jqLiteMode) return; // don't run in jQuery

var eventFn;
var window = {
document: {},
Expand Down Expand Up @@ -1260,7 +1283,7 @@ describe('jqLite', function() {
});

it('should fire mouseenter when coming from outside the browser window', function() {
if (window.jQuery) return;
if (!_jqLiteMode) return;

setup('<div>root<p>parent<span>child</span></p><ul></ul></div>', 'p', 'span');

Expand All @@ -1279,7 +1302,7 @@ describe('jqLite', function() {
});

it('should fire the mousenter on SVG elements', function() {
if (window.jQuery) return;
if (!_jqLiteMode) return;

setup(
'<div>' +
Expand All @@ -1301,29 +1324,28 @@ describe('jqLite', function() {
});
});

// Only run this test for jqLite and not normal jQuery
if (_jqLiteMode) {
it('should throw an error if eventData or a selector is passed', function() {
var elm = jqLite(a),
anObj = {},
aString = '',
aValue = 45,
callback = function() {};
it('should throw an error if eventData or a selector is passed', function() {
if (!_jqLiteMode) return;

expect(function() {
elm.on('click', anObj, callback);
}).toThrowMinErr('jqLite', 'onargs');
var elm = jqLite(a),
anObj = {},
aString = '',
aValue = 45,
callback = function() {};

expect(function() {
elm.on('click', null, aString, callback);
}).toThrowMinErr('jqLite', 'onargs');
expect(function() {
elm.on('click', anObj, callback);
}).toThrowMinErr('jqLite', 'onargs');

expect(function() {
elm.on('click', aValue, callback);
}).toThrowMinErr('jqLite', 'onargs');
expect(function() {
elm.on('click', null, aString, callback);
}).toThrowMinErr('jqLite', 'onargs');

});
}
expect(function() {
elm.on('click', aValue, callback);
}).toThrowMinErr('jqLite', 'onargs');

});
});


Expand Down Expand Up @@ -1554,11 +1576,6 @@ describe('jqLite', function() {


describe('native listener deregistration', function() {
var jQueryVersionString = window.jQuery && window.jQuery.fn.jquery;
var jQueryMajor = jQueryVersionString && Number(jQueryVersionString.split('.')[0]);
var jQueryMinor = jQueryVersionString && Number(jQueryVersionString.split('.')[1]);
var jQuery21 = jQueryMajor === 2 && jQueryMinor === 1;

it('should deregister the native listener when all jqLite listeners for given type are gone ' +
'after off("eventName", listener) call', function() {
var aElem = jqLite(a);
Expand All @@ -1571,7 +1588,7 @@ describe('jqLite', function() {

// jQuery <2.2 passes the non-needed `false` useCapture parameter.
// See https://github.com/jquery/jquery/issues/2199 for details.
if (jQuery21) {
if (isJQuery21()) {
expect(addEventListenerSpy).toHaveBeenCalledOnceWith('click', jasmine.any(Function), false);
} else {
expect(addEventListenerSpy).toHaveBeenCalledOnceWith('click', jasmine.any(Function));
Expand All @@ -1580,7 +1597,7 @@ describe('jqLite', function() {
expect(removeEventListenerSpy).not.toHaveBeenCalled();

aElem.off('click', jqLiteListener);
if (jQuery21) {
if (isJQuery21()) {
expect(removeEventListenerSpy).toHaveBeenCalledOnceWith('click', nativeListenerFn, false);
} else {
expect(removeEventListenerSpy).toHaveBeenCalledOnceWith('click', nativeListenerFn);
Expand All @@ -1596,7 +1613,7 @@ describe('jqLite', function() {
var nativeListenerFn;

aElem.on('click', function() {});
if (jQuery21) {
if (isJQuery21()) {
expect(addEventListenerSpy).toHaveBeenCalledOnceWith('click', jasmine.any(Function), false);
} else {
expect(addEventListenerSpy).toHaveBeenCalledOnceWith('click', jasmine.any(Function));
Expand All @@ -1605,7 +1622,7 @@ describe('jqLite', function() {
expect(removeEventListenerSpy).not.toHaveBeenCalled();

aElem.off('click');
if (jQuery21) {
if (isJQuery21()) {
expect(removeEventListenerSpy).toHaveBeenCalledOnceWith('click', nativeListenerFn, false);
} else {
expect(removeEventListenerSpy).toHaveBeenCalledOnceWith('click', nativeListenerFn);
Expand All @@ -1621,7 +1638,7 @@ describe('jqLite', function() {
var nativeListenerFn;

aElem.on('click', function() {});
if (jQuery21) {
if (isJQuery21()) {
expect(addEventListenerSpy).toHaveBeenCalledOnceWith('click', jasmine.any(Function), false);
} else {
expect(addEventListenerSpy).toHaveBeenCalledOnceWith('click', jasmine.any(Function));
Expand All @@ -1630,7 +1647,7 @@ describe('jqLite', function() {
addEventListenerSpy.calls.reset();

aElem.on('dblclick', function() {});
if (jQuery21) {
if (isJQuery21()) {
expect(addEventListenerSpy).toHaveBeenCalledOnceWith('dblclick', nativeListenerFn, false);
} else {
expect(addEventListenerSpy).toHaveBeenCalledOnceWith('dblclick', nativeListenerFn);
Expand All @@ -1640,7 +1657,7 @@ describe('jqLite', function() {

aElem.off('click dblclick');

if (jQuery21) {
if (isJQuery21()) {
expect(removeEventListenerSpy).toHaveBeenCalledWith('click', nativeListenerFn, false);
expect(removeEventListenerSpy).toHaveBeenCalledWith('dblclick', nativeListenerFn, false);
} else {
Expand All @@ -1659,7 +1676,7 @@ describe('jqLite', function() {
var nativeListenerFn;

aElem.on('click', function() {});
if (jQuery21) {
if (isJQuery21()) {
expect(addEventListenerSpy).toHaveBeenCalledOnceWith('click', jasmine.any(Function), false);
} else {
expect(addEventListenerSpy).toHaveBeenCalledOnceWith('click', jasmine.any(Function));
Expand All @@ -1668,15 +1685,15 @@ describe('jqLite', function() {
addEventListenerSpy.calls.reset();

aElem.on('dblclick', function() {});
if (jQuery21) {
if (isJQuery21()) {
expect(addEventListenerSpy).toHaveBeenCalledOnceWith('dblclick', nativeListenerFn, false);
} else {
expect(addEventListenerSpy).toHaveBeenCalledOnceWith('dblclick', nativeListenerFn);
}

aElem.off();

if (jQuery21) {
if (isJQuery21()) {
expect(removeEventListenerSpy).toHaveBeenCalledWith('click', nativeListenerFn, false);
expect(removeEventListenerSpy).toHaveBeenCalledWith('dblclick', nativeListenerFn, false);
} else {
Expand All @@ -1688,16 +1705,15 @@ describe('jqLite', function() {
});


// Only run this test for jqLite and not normal jQuery
if (_jqLiteMode) {
it('should throw an error if a selector is passed', function() {
var aElem = jqLite(a);
aElem.on('click', noop);
expect(function() {
aElem.off('click', noop, '.test');
}).toThrowError(/\[jqLite:offargs\]/);
});
}
it('should throw an error if a selector is passed', function() {
if (!_jqLiteMode) return;

var aElem = jqLite(a);
aElem.on('click', noop);
expect(function() {
aElem.off('click', noop, '.test');
}).toThrowError(/\[jqLite:offargs\]/);
});
});

describe('one', function() {
Expand Down