From 3b05c484af074f0c7c050b0f9824099d627e178f Mon Sep 17 00:00:00 2001 From: Martin Staffa Date: Mon, 14 Mar 2016 18:07:09 +0100 Subject: [PATCH] fix(ngOptions): set select value when model matches disabled option When ngModel is set to a value that matches a disabled option, ngOptions will now select the option in the select element, which will set select.val() to the option hash value and visually show the option value / label as selected in the select box. Previously, disabled options forced the unknown value. The previous behavior is inconsistent with both default HTML behavior and select with ngModel but without ngOptions. Both allow disabled values to be selected programmatically. A common use case for this behavior is an option that was previously valid, but has been disabled, and cannot be selected again. This commit removes a duplicate test, and all other tests that previously checked that disabled options are not set have been adjusted to the ensure the opposite. Fixes #12756 --- src/ng/directive/ngOptions.js | 4 +- test/ng/directive/ngOptionsSpec.js | 63 +++++++++--------------------- 2 files changed, 21 insertions(+), 46 deletions(-) diff --git a/src/ng/directive/ngOptions.js b/src/ng/directive/ngOptions.js index cbd0877f04bb..2283a26c5dea 100644 --- a/src/ng/directive/ngOptions.js +++ b/src/ng/directive/ngOptions.js @@ -467,7 +467,7 @@ var ngOptionsDirective = ['$compile', '$parse', function($compile, $parse) { selectCtrl.writeValue = function writeNgOptionsValue(value) { var option = options.getOptionFromViewValue(value); - if (option && !option.disabled) { + if (option) { // Don't update the option when it is already selected. // For example, the browser will select the first option by default. In that case, // most properties are set automatically - except the `selected` attribute, which we @@ -529,7 +529,7 @@ var ngOptionsDirective = ['$compile', '$parse', function($compile, $parse) { if (value) { value.forEach(function(item) { var option = options.getOptionFromViewValue(item); - if (option && !option.disabled) option.element.selected = true; + if (option) option.element.selected = true; }); } }; diff --git a/test/ng/directive/ngOptionsSpec.js b/test/ng/directive/ngOptionsSpec.js index 015a3f6d31df..232f9193cbfc 100644 --- a/test/ng/directive/ngOptionsSpec.js +++ b/test/ng/directive/ngOptionsSpec.js @@ -771,7 +771,7 @@ describe('ngOptions', function() { }); - it('should not select disabled options when model changes', function() { + it('should select disabled options when model changes', function() { scope.options = [ { name: 'white', value: '#FFFFFF' }, { name: 'one', value: 1, unavailable: true }, @@ -792,11 +792,14 @@ describe('ngOptions', function() { scope.$apply('selected = 1'); options = element.find('option'); - expect(element.val()).toEqualUnknownValue('?'); - expect(options.length).toEqual(5); - expect(options.eq(0).prop('selected')).toEqual(true); + // jQuery returns null for val() when the option is disabled, see + // https://bugs.jquery.com/ticket/13097 + expect(element[0].value).toBe('number:1'); + expect(options.length).toEqual(4); + expect(options.eq(0).prop('selected')).toEqual(false); + expect(options.eq(1).prop('selected')).toEqual(true); expect(options.eq(2).prop('selected')).toEqual(false); - expect(options.eq(4).prop('selected')).toEqual(false); + expect(options.eq(3).prop('selected')).toEqual(false); }); @@ -816,11 +819,14 @@ describe('ngOptions', function() { scope.$apply('selected = 1'); var options = element.find('option'); - expect(element.val()).toEqualUnknownValue('?'); - expect(options.length).toEqual(5); - expect(options.eq(0).prop('selected')).toEqual(true); + // jQuery returns null for val() when the option is disabled, see + // https://bugs.jquery.com/ticket/13097 + expect(element[0].value).toBe('number:1'); + expect(options.length).toEqual(4); + expect(options.eq(0).prop('selected')).toEqual(false); + expect(options.eq(1).prop('selected')).toEqual(true); expect(options.eq(2).prop('selected')).toEqual(false); - expect(options.eq(4).prop('selected')).toEqual(false); + expect(options.eq(3).prop('selected')).toEqual(false); // Now enable that option scope.$apply(function() { @@ -861,7 +867,7 @@ describe('ngOptions', function() { }); - it('should not select disabled options when model changes', function() { + it('should select disabled options when model changes', function() { scope.options = [ { name: 'a', value: 0 }, { name: 'b', value: 1, unavailable: true }, @@ -886,14 +892,14 @@ describe('ngOptions', function() { scope.$apply('selected = [1,3]'); options = element.find('option'); expect(options.eq(0).prop('selected')).toEqual(false); - expect(options.eq(1).prop('selected')).toEqual(false); + expect(options.eq(1).prop('selected')).toEqual(true); expect(options.eq(2).prop('selected')).toEqual(false); expect(options.eq(3).prop('selected')).toEqual(true); // Now only select the disabled option scope.$apply('selected = [1]'); expect(options.eq(0).prop('selected')).toEqual(false); - expect(options.eq(1).prop('selected')).toEqual(false); + expect(options.eq(1).prop('selected')).toEqual(true); expect(options.eq(2).prop('selected')).toEqual(false); expect(options.eq(3).prop('selected')).toEqual(false); }); @@ -917,7 +923,7 @@ describe('ngOptions', function() { var options = element.find('option'); expect(options.eq(0).prop('selected')).toEqual(false); - expect(options.eq(1).prop('selected')).toEqual(false); + expect(options.eq(1).prop('selected')).toEqual(true); expect(options.eq(2).prop('selected')).toEqual(false); expect(options.eq(3).prop('selected')).toEqual(false); @@ -2535,37 +2541,6 @@ describe('ngOptions', function() { expect(element.find('option')[1].selected).toBeTruthy(); }); - it('should not write disabled selections from model', function() { - scope.selected = [30]; - scope.options = [ - { name: 'white', value: '#FFFFFF' }, - { name: 'one', value: 1, unavailable: true }, - { name: 'notTrue', value: false }, - { name: 'thirty', value: 30, unavailable: false } - ]; - createSelect({ - 'ng-options': 'o.value as o.name disable when o.unavailable for o in options', - 'ng-model': 'selected', - 'multiple': true - }); - - var options = element.find('option'); - - expect(options.eq(0).prop('selected')).toEqual(false); - expect(options.eq(1).prop('selected')).toEqual(false); - expect(options.eq(2).prop('selected')).toEqual(false); - expect(options.eq(3).prop('selected')).toEqual(true); - - scope.$apply(function() { - scope.selected.push(1); - }); - - expect(options.eq(0).prop('selected')).toEqual(false); - expect(options.eq(1).prop('selected')).toEqual(false); - expect(options.eq(2).prop('selected')).toEqual(false); - expect(options.eq(3).prop('selected')).toEqual(true); - }); - it('should update model on change', function() { createMultiSelect();