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

fix(select): Fix several issues when moving options between groups #10166

Closed
wants to merge 1 commit into from
Closed

fix(select): Fix several issues when moving options between groups #10166

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
21 changes: 13 additions & 8 deletions src/ng/directive/select.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -679,18 +679,23 @@ var selectDirective = ['$compile', '$parse', function($compile, $parse) {
updateLabelMap(labelMap, option.label, false);
option.element.remove();
}
forEach(labelMap, function(count, label) {
if (count > 0) {
selectCtrl.addOption(label);
} else if (count < 0) {
selectCtrl.removeOption(label);
}
});
}
// remove any excessive OPTGROUPs from select
while (optionGroupsCache.length > groupIndex) {
optionGroupsCache.pop()[0].element.remove();
// remove all the labels in the option group
optionGroup = optionGroupsCache.pop();
for (index = 1; index < optionGroup.length; ++index) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add a comment here ala "Make sure the labels for removed option groups are updated"? select is a little short on comments, I'd rather have it a little more verbose

updateLabelMap(labelMap, optionGroup[index].label, false);
}
optionGroup[0].element.remove();
}
forEach(labelMap, function(count, label) {
if (count > 0) {
selectCtrl.addOption(label);
} else if (count < 0) {
selectCtrl.removeOption(label);
}
});
}
}
}
Expand Down
182 changes: 182 additions & 0 deletions test/ng/directive/selectSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,23 @@ describe('select', function() {
return equals(expectedValues, actualValues);
},

toEqualSelectWithOptions: function(expected) {
var actualValues = {};
var optionGroup;

forEach(this.actual.find('option'), function(option) {
optionGroup = option.parentNode.label || '';
actualValues[optionGroup] = actualValues[optionGroup] || [];
actualValues[optionGroup].push(option.label);
});

this.message = function() {
return 'Expected ' + toJson(actualValues) + ' to equal ' + toJson(expected) + '.';
};

return equals(expected, actualValues);
},

toEqualOption: function(value, text, label) {
var errors = [];
if (this.actual.attr('value') !== value) {
Expand Down Expand Up @@ -447,6 +464,7 @@ describe('select', function() {
expect(selectCtrl.hasOption('r2d2')).toBe(true);
});


it('should return false for options popped via ngOptions', function() {
scope.robots = [
{value: 1, label: 'c3p0'},
Expand All @@ -467,6 +485,7 @@ describe('select', function() {
expect(selectCtrl.hasOption('r2d2')).toBe(false);
});


it('should return true for options added via ngOptions', function() {
scope.robots = [
{value: 2, label: 'r2d2'}
Expand All @@ -485,6 +504,169 @@ describe('select', function() {
expect(selectCtrl.hasOption('c3p0')).toBe(true);
expect(selectCtrl.hasOption('r2d2')).toBe(true);
});


it('should keep all the options when changing the model', function() {
Copy link
Contributor

Choose a reason for hiding this comment

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

the assertions in this test, don't assert that all options are being kept. either change assertions or the description.

compile('<select ng-model="mySelect" ng-options="o for o in [\'A\',\'B\',\'C\']"></select>');
var selectCtrl = element.data().$selectController;
scope.$apply(function() {
scope.mySelect = 'C';
});
expect(selectCtrl.hasOption('A')).toBe(true);
expect(selectCtrl.hasOption('B')).toBe(true);
expect(selectCtrl.hasOption('C')).toBe(true);
expect(element).toEqualSelectWithOptions({'': ['A', 'B', 'C']});
});
Copy link
Contributor

Choose a reason for hiding this comment

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

two empty blanks betweets its please. one day we'll have a jscs rule for this :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this test is based on #10161 (at 1.3 this is working, but I will need to backport this to 1.2). Anyhow, will add the extra checks



it('should be able to detect when elements move from a previous group', function() {
scope.values = [
{name: 'A'},
{name: 'B', group: 'first'},
{name: 'C', group: 'first'},
{name: 'D', group: 'first'},
{name: 'E', group: 'second'}
];

compile('<select ng-model="mySelect" ng-options="item.name group by item.group for item in values"></select>');
var selectCtrl = element.data().$selectController;
Copy link
Contributor

Choose a reason for hiding this comment

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

element.controller would be nicer

Copy link
Contributor Author

Choose a reason for hiding this comment

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

agree that it would be nicer, but right now controllers are only exposed as
$element.data('$' + directive.name + 'Controller', controllerInstance.instance)

Copy link
Contributor

Choose a reason for hiding this comment

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

You can achieve this with

var selectCtrl = element.controller('select');


scope.$apply(function() {
scope.values[3] = {name: 'D', group: 'second'};
scope.values.shift();
});
expect(selectCtrl.hasOption('A')).toBe(false);
expect(selectCtrl.hasOption('B')).toBe(true);
expect(selectCtrl.hasOption('C')).toBe(true);
expect(selectCtrl.hasOption('D')).toBe(true);
Copy link
Contributor

Choose a reason for hiding this comment

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

this assertion is pretty weak. don't you want to assert that the d is in the right group to?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we do not store this at the controller level, will add a test on the DOM to check for this

expect(selectCtrl.hasOption('E')).toBe(true);
expect(element).toEqualSelectWithOptions({'': [''], 'first':['B', 'C'], 'second': ['D', 'E']});
});


it('should be able to detect when elements move from a following group', function() {
scope.values = [
{name: 'A'},
{name: 'B', group: 'first'},
{name: 'C', group: 'first'},
{name: 'D', group: 'second'},
{name: 'E', group: 'second'}
];

compile('<select ng-model="mySelect" ng-options="item.name group by item.group for item in values"></select>');
var selectCtrl = element.data().$selectController;
Copy link
Contributor

Choose a reason for hiding this comment

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

same issues as above


scope.$apply(function() {
scope.values[3].group = 'first';
scope.values.shift();
});
expect(selectCtrl.hasOption('A')).toBe(false);
expect(selectCtrl.hasOption('B')).toBe(true);
expect(selectCtrl.hasOption('C')).toBe(true);
expect(selectCtrl.hasOption('D')).toBe(true);
expect(selectCtrl.hasOption('E')).toBe(true);
expect(element).toEqualSelectWithOptions({'': [''], 'first':['B', 'C', 'D'], 'second': ['E']});
});


it('should be able to detect when an element is replaced with an element from a previous group', function() {
scope.values = [
{name: 'A'},
{name: 'B', group: 'first'},
{name: 'C', group: 'first'},
{name: 'D', group: 'first'},
{name: 'E', group: 'second'},
{name: 'F', group: 'second'}
];

compile('<select ng-model="mySelect" ng-options="item.name group by item.group for item in values"></select>');
var selectCtrl = element.data().$selectController;

scope.$apply(function() {
scope.values[3].group = 'second';
scope.values.pop();
});
expect(selectCtrl.hasOption('A')).toBe(true);
expect(selectCtrl.hasOption('B')).toBe(true);
expect(selectCtrl.hasOption('C')).toBe(true);
expect(selectCtrl.hasOption('D')).toBe(true);
expect(selectCtrl.hasOption('E')).toBe(true);
expect(selectCtrl.hasOption('F')).toBe(false);
expect(element).toEqualSelectWithOptions({'': ['', 'A'], 'first':['B', 'C'], 'second': ['D', 'E']});
});


it('should be able to detect when element is replaced with an element from a following group', function() {
scope.values = [
{name: 'A'},
{name: 'B', group: 'first'},
{name: 'C', group: 'first'},
{name: 'D', group: 'second'},
{name: 'E', group: 'second'}
];

compile('<select ng-model="mySelect" ng-options="item.name group by item.group for item in values"></select>');
var selectCtrl = element.data().$selectController;
Copy link
Contributor

Choose a reason for hiding this comment

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

more


scope.$apply(function() {
scope.values[3].group = 'first';
scope.values.splice(2, 1);
});
expect(selectCtrl.hasOption('A')).toBe(true);
expect(selectCtrl.hasOption('B')).toBe(true);
expect(selectCtrl.hasOption('C')).toBe(false);
expect(selectCtrl.hasOption('D')).toBe(true);
Copy link
Contributor

Choose a reason for hiding this comment

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

more

expect(selectCtrl.hasOption('E')).toBe(true);
expect(element).toEqualSelectWithOptions({'': ['', 'A'], 'first':['B', 'D'], 'second': ['E']});
});


it('should be able to detect when an element is removed', function() {
scope.values = [
{name: 'A'},
{name: 'B', group: 'first'},
{name: 'C', group: 'first'},
{name: 'D', group: 'second'},
{name: 'E', group: 'second'}
];

compile('<select ng-model="mySelect" ng-options="item.name group by item.group for item in values"></select>');
var selectCtrl = element.data().$selectController;

scope.$apply(function() {
scope.values.splice(3, 1);
});
expect(selectCtrl.hasOption('A')).toBe(true);
expect(selectCtrl.hasOption('B')).toBe(true);
expect(selectCtrl.hasOption('C')).toBe(true);
expect(selectCtrl.hasOption('D')).toBe(false);
expect(selectCtrl.hasOption('E')).toBe(true);
expect(element).toEqualSelectWithOptions({'': ['', 'A'], 'first':['B', 'C'], 'second': ['E']});
});


it('should be able to detect when a group is removed', function() {
scope.values = [
{name: 'A'},
{name: 'B', group: 'first'},
{name: 'C', group: 'first'},
{name: 'D', group: 'second'},
{name: 'E', group: 'second'}
];

compile('<select ng-model="mySelect" ng-options="item.name group by item.group for item in values"></select>');
var selectCtrl = element.data().$selectController;

scope.$apply(function() {
scope.values.splice(3, 2);
});
expect(selectCtrl.hasOption('A')).toBe(true);
expect(selectCtrl.hasOption('B')).toBe(true);
expect(selectCtrl.hasOption('C')).toBe(true);
expect(selectCtrl.hasOption('D')).toBe(false);
expect(selectCtrl.hasOption('E')).toBe(false);
Copy link
Contributor

Choose a reason for hiding this comment

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

do you actually want to make this assertions on the controller level? why not on the DOM level?

what if you created a helper function that would assert the state of the select DOM tree. something like:

expect(element).toBeSelectWithOptions(['D', 'E', 'A']) I think we might even have such helper somewhere.

For groups it could look like:

expect(element).toBeSelectWithOptions({group1: ['D', 'E'], group2: ['A']})

Copy link
Contributor Author

Choose a reason for hiding this comment

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

at the DOM level we are doing almost always the right thing (at least with 1.3). The issue is that when the controller and the DOM are not in sync, then we run into weird interactions like #10161

expect(element).toEqualSelectWithOptions({'': ['', 'A'], 'first':['B', 'C']});
});
});
});
});
Expand Down