Skip to content

Commit ae9e23d

Browse files
NarretzNarretz
committed
fix(ngOptions, select): if select is "required", set error when unknown option selected
With this patch, a regular / ngOptions select with the "required" attribute will now add the "required" error to the ngModel if the *unknown" option is selected. Previously, the error was only set if the *empty* option was selected. The unknown option is selected if the model is set to a value that does not match any option. If the select has an empty option, this option is selected when the model is `null` or `undefined`. For both, "required" means that the select is invalid, as no correct option selection is provided. Closes angular#13354 Fixes angular#13172
1 parent 8eb925d commit ae9e23d

File tree

4 files changed

+170
-53
lines changed

4 files changed

+170
-53
lines changed

‎src/ng/directive/ngOptions.js

+3-1
Original file line numberDiff line numberDiff line change
@@ -633,6 +633,7 @@ var ngOptionsDirective = ['$compile', '$document', '$parse', function($compile,
633633

634634
function updateOptions() {
635635
var previousValue = options && selectCtrl.readValue();
636+
var previouslyEmpty = ngModelCtrl.$isEmpty(previousValue);
636637

637638
// We must remove all current options, but cannot simply set innerHTML = null
638639
// since the providedEmptyOption might have an ngIf on it that inserts comments which we
@@ -696,7 +697,7 @@ var ngOptionsDirective = ['$compile', '$document', '$parse', function($compile,
696697
ngModelCtrl.$render();
697698

698699
// Check to see if the value has changed due to the update to the options
699-
if (!ngModelCtrl.$isEmpty(previousValue)) {
700+
if (!previouslyEmpty) {
700701
var nextValue = selectCtrl.readValue();
701702
var isNotPrimitive = ngOptions.trackBy || multiple;
702703
if (isNotPrimitive ? !equals(previousValue, nextValue) : previousValue !== nextValue) {
@@ -705,6 +706,7 @@ var ngOptionsDirective = ['$compile', '$document', '$parse', function($compile,
705706
}
706707
}
707708

709+
ngModelCtrl.$validate();
708710
}
709711
}
710712

‎src/ng/directive/select.js

+15
Original file line numberDiff line numberDiff line change
@@ -650,6 +650,21 @@ var selectDirective = function() {
650650
return !value || value.length === 0;
651651
};
652652

653+
} else {
654+
655+
var originalIsEmpty = ngModelCtrl.$isEmpty;
656+
657+
// A single select is also considered empty when the unknown option is present, or when
658+
// the empty option is selected
659+
ngModelCtrl.$isEmpty = function(value) {
660+
661+
return originalIsEmpty(value) ||
662+
!!selectCtrl.unknownOption.parent().length ||
663+
// When the empty option is selected from the scope, then the viewValue can be different
664+
// than '', and the originalIsEmpty fn will not catch this case
665+
(selectCtrl.hasEmptyOption &&
666+
element[0].options[element[0].selectedIndex] === selectCtrl.emptyOption[0]);
667+
};
653668
}
654669
}
655670

‎test/ng/directive/ngOptionsSpec.js

+91-15
Original file line numberDiff line numberDiff line change
@@ -2,12 +2,13 @@
22

33
describe('ngOptions', function() {
44

5-
var scope, formElement, element, $compile, linkLog;
5+
var scope, formElement, element, $compile, linkLog, ngModelCtrl;
66

77
function compile(html) {
88
formElement = jqLite('<form name="form">' + html + '</form>');
99
element = formElement.find('select');
1010
$compile(formElement)(scope);
11+
ngModelCtrl = element.controller('ngModel');
1112
scope.$apply();
1213
}
1314

@@ -181,6 +182,7 @@ describe('ngOptions', function() {
181182
afterEach(function() {
182183
scope.$destroy(); //disables unknown option work during destruction
183184
dealoc(formElement);
185+
ngModelCtrl = null;
184186
});
185187

186188
function createSelect(attrs, blank, unknown) {
@@ -2925,45 +2927,119 @@ describe('ngOptions', function() {
29252927
});
29262928

29272929

2928-
describe('ngRequired', function() {
2930+
describe('required state', function() {
29292931

2930-
it('should allow bindings on ngRequired', function() {
2932+
it('should set the error if the empty option is selected', function() {
2933+
createSelect({
2934+
'ng-model': 'selection',
2935+
'ng-options': 'item for item in values',
2936+
'required': ''
2937+
}, true);
2938+
2939+
scope.$apply(function() {
2940+
scope.values = ['a', 'b'];
2941+
scope.selection = scope.values[0];
2942+
});
2943+
expect(element).toBeValid();
2944+
expect(ngModelCtrl.$error.required).toBeFalsy();
2945+
2946+
var options = element.find('option');
2947+
2948+
// view -> model
2949+
browserTrigger(options[0], 'click');
2950+
expect(element).toBeInvalid();
2951+
expect(ngModelCtrl.$error.required).toBeTruthy();
2952+
2953+
browserTrigger(options[1], 'click');
2954+
expect(element).toBeValid();
2955+
expect(ngModelCtrl.$error.required).toBeFalsy();
2956+
2957+
// model -> view
2958+
scope.$apply('selection = "unmatched value"');
2959+
expect(options[0]).toBeMarkedAsSelected();
2960+
expect(element).toBeInvalid();
2961+
expect(ngModelCtrl.$error.required).toBeTruthy();
2962+
});
2963+
2964+
it('should validate with unknown option and required', function() {
2965+
createSelect({
2966+
'ng-model': 'value',
2967+
'ng-options': 'item.name for item in values',
2968+
'required': ''
2969+
}, false, true);
2970+
2971+
scope.$apply(function() {
2972+
scope.values = [{name: 'A', id: 1}, {name: 'B', id: 2}];
2973+
scope.value = scope.values[0];
2974+
});
2975+
expect(element).toBeValid();
2976+
2977+
scope.$apply('value = "unmatched value"');
2978+
expect(element).toBeInvalid();
2979+
2980+
var options = element.find('option');
2981+
browserTrigger(options[1], 'click');
2982+
expect(element).toBeValid();
2983+
});
2984+
2985+
it('should validate with empty option and bound ngRequired', function() {
29312986
createSelect({
29322987
'ng-model': 'value',
29332988
'ng-options': 'item.name for item in values',
29342989
'ng-required': 'required'
29352990
}, true);
29362991

2937-
29382992
scope.$apply(function() {
29392993
scope.values = [{name: 'A', id: 1}, {name: 'B', id: 2}];
29402994
scope.required = false;
29412995
});
29422996

2943-
element.val('');
2944-
browserTrigger(element, 'change');
2997+
var options = element.find('option');
2998+
2999+
browserTrigger(options[0], 'click');
29453000
expect(element).toBeValid();
29463001

2947-
scope.$apply(function() {
2948-
scope.required = true;
2949-
});
3002+
scope.$apply('required = true');
29503003
expect(element).toBeInvalid();
29513004

3005+
scope.$apply('value = values[0]');
3006+
expect(element).toBeValid();
3007+
3008+
browserTrigger(options[0], 'click');
3009+
expect(element).toBeInvalid();
3010+
3011+
scope.$apply('required = false');
3012+
expect(element).toBeValid();
3013+
});
3014+
3015+
it('should validate with unknown option and bound ngRequired', function() {
3016+
createSelect({
3017+
'ng-model': 'value',
3018+
'ng-options': 'item.name for item in values',
3019+
'ng-required': 'required'
3020+
}, false, true);
3021+
29523022
scope.$apply(function() {
3023+
scope.values = [{name: 'A', id: 1}, {name: 'B', id: 2}];
29533024
scope.value = scope.values[0];
3025+
scope.required = true;
29543026
});
3027+
3028+
var options = element.find('option');
29553029
expect(element).toBeValid();
29563030

2957-
element.val('');
2958-
browserTrigger(element, 'change');
3031+
scope.$apply('value = "asdf"');
29593032
expect(element).toBeInvalid();
29603033

2961-
scope.$apply(function() {
2962-
scope.required = false;
2963-
});
3034+
browserTrigger(options[1], 'click');
29643035
expect(element).toBeValid();
2965-
});
29663036

3037+
scope.$apply('required = false');
3038+
expect(element).toBeValid();
3039+
3040+
scope.$apply('value = "asdf"');
3041+
expect(element).toBeValid();
3042+
});
29673043

29683044
it('should treat an empty array as invalid when `multiple` attribute used', function() {
29693045
createSelect({

‎test/ng/directive/selectSpec.js

+61-37
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ describe('select', function() {
77
formElement = jqLite('<form name="form">' + html + '</form>');
88
element = formElement.find('select');
99
$compile(formElement)(scope);
10+
ngModelCtrl = element.controller('ngModel');
1011
scope.$digest();
1112
}
1213

@@ -79,6 +80,7 @@ describe('select', function() {
7980
afterEach(function() {
8081
scope.$destroy(); //disables unknown option work during destruction
8182
dealoc(formElement);
83+
ngModelCtrl = null;
8284
});
8385

8486

@@ -190,54 +192,76 @@ describe('select', function() {
190192
});
191193

192194

193-
it('should require', function() {
194-
compile(
195-
'<select name="select" ng-model="selection" required ng-change="change()">' +
196-
'<option value=""></option>' +
197-
'<option value="c">C</option>' +
198-
'</select>');
195+
describe('required state', function() {
199196

200-
scope.change = function() {
201-
scope.log += 'change;';
202-
};
197+
it('should set the error if the empty option is selected', function() {
198+
compile(
199+
'<select name="select" ng-model="selection" required>' +
200+
'<option value=""></option>' +
201+
'<option value="c">C</option>' +
202+
'</select>');
203203

204-
scope.$apply(function() {
205-
scope.log = '';
206-
scope.selection = 'c';
207-
});
204+
scope.$apply(function() {
205+
scope.selection = 'c';
206+
});
208207

209-
expect(scope.form.select.$error.required).toBeFalsy();
210-
expect(element).toBeValid();
211-
expect(element).toBePristine();
208+
expect(element).toBeValid();
209+
expect(ngModelCtrl.$error.required).toBeFalsy();
212210

213-
scope.$apply(function() {
214-
scope.selection = '';
211+
var options = element.find('option');
212+
213+
// view -> model
214+
browserTrigger(options[0], 'click');
215+
expect(element).toBeInvalid();
216+
expect(ngModelCtrl.$error.required).toBeTruthy();
217+
218+
browserTrigger(options[1], 'click');
219+
expect(element).toBeValid();
220+
expect(ngModelCtrl.$error.required).toBeFalsy();
221+
222+
// model -> view
223+
scope.$apply('selection = "unmatched value"');
224+
options = element.find('option');
225+
expect(options[0]).toBeMarkedAsSelected();
226+
expect(element).toBeInvalid();
227+
expect(ngModelCtrl.$error.required).toBeTruthy();
215228
});
216229

217-
expect(scope.form.select.$error.required).toBeTruthy();
218-
expect(element).toBeInvalid();
219-
expect(element).toBePristine();
220-
expect(scope.log).toEqual('');
221230

222-
element[0].value = 'c';
223-
browserTrigger(element, 'change');
224-
expect(element).toBeValid();
225-
expect(element).toBeDirty();
226-
expect(scope.log).toEqual('change;');
227-
});
231+
it('should set the error if the unknown option is selected', function() {
232+
compile(
233+
'<select name="select" ng-model="selection" required>' +
234+
'<option value="a">A</option>' +
235+
'<option value="b">B</option>' +
236+
'</select>');
228237

238+
scope.$apply(function() {
239+
scope.selection = 'a';
240+
});
241+
expect(element).toBeValid();
229242

230-
it('should not be invalid if no require', function() {
231-
compile(
232-
'<select name="select" ng-model="selection">' +
233-
'<option value=""></option>' +
234-
'<option value="c">C</option>' +
235-
'</select>');
243+
scope.$apply('selection = "unmatched value"');
244+
expect(element).toBeInvalid();
245+
expect(ngModelCtrl.$error.required).toBeTruthy();
246+
247+
var options = element.find('option');
248+
browserTrigger(options[1], 'click');
249+
expect(element).toBeValid();
250+
});
236251

237-
expect(element).toBeValid();
238-
expect(element).toBePristine();
239-
});
240252

253+
it('should not be invalid if no required attribute is present', function() {
254+
compile(
255+
'<select name="select" ng-model="selection">' +
256+
'<option value=""></option>' +
257+
'<option value="c">C</option>' +
258+
'</select>');
259+
260+
expect(element).toBeValid();
261+
expect(element).toBePristine();
262+
});
263+
264+
});
241265

242266
it('should work with repeated value options', function() {
243267
scope.robots = ['c3p0', 'r2d2'];

0 commit comments

Comments
 (0)