From 49c65f13dfd258b6d35a4b847e44dfd5c00f5364 Mon Sep 17 00:00:00 2001 From: Erin Altenhof-Long Date: Wed, 16 Jul 2014 11:30:19 -0700 Subject: [PATCH 1/5] revert(select): avoid checking option element selected properties in render This reverts commit dc149de9364c66b988f169f67cad39577ba43434. That commit fixes a bug caused by Firefox updating `select.value` on hover. However, it causes other bugs with select including the issue described in #7715. This issue details how selects with a blank disabled option skip to the second option. We filed a bug with Firefox for the problematic behavior the reverted commit addresses https://bugzilla.mozilla.org/show_bug.cgi?id=1039047, and alternate Angular fixes are being investigated. The test introduced in this commit does test a valid case and is therefore not reverted. Closes #7715 #7855 --- src/ng/directive/select.js | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/src/ng/directive/select.js b/src/ng/directive/select.js index 886aae20306a..03c65992f0a9 100644 --- a/src/ng/directive/select.js +++ b/src/ng/directive/select.js @@ -405,12 +405,6 @@ var selectDirective = ['$compile', '$parse', function($compile, $parse) { value = valueFn(scope, locals); } } - // Update the null option's selected property here so $render cleans it up correctly - if (optionGroupsCache[0].length > 1) { - if (optionGroupsCache[0][1].id !== key) { - optionGroupsCache[0][1].selected = false; - } - } } ctrl.$setViewValue(value); }); @@ -548,7 +542,7 @@ var selectDirective = ['$compile', '$parse', function($compile, $parse) { lastElement.val(existingOption.id = option.id); } // lastElement.prop('selected') provided by jQuery has side-effects - if (existingOption.selected !== option.selected) { + if (lastElement[0].selected !== option.selected) { lastElement.prop('selected', (existingOption.selected = option.selected)); if (msie) { // See #7692 From 7571f61c55dd33d457e38852184379fbba9b48c6 Mon Sep 17 00:00:00 2001 From: Erin Altenhof-Long Date: Wed, 16 Jul 2014 11:43:47 -0700 Subject: [PATCH 2/5] test(select): add test cases for selects with blank disabled options An earlier commit dc149de9364c66b988f169f67cad39577ba43434 caused an error where the first option of a select would be skipped over if it had a blank disabled value. These tests demonstrate that with that commit in place, blank disabled options are skipped in a select. When the commit is reverted, the correct behavior is seen that the blank disabled option is still selected in both selects marked with required and those that have optional choices. Relates to #7715 --- test/ng/directive/selectSpec.js | 40 +++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+) diff --git a/test/ng/directive/selectSpec.js b/test/ng/directive/selectSpec.js index 9ce01704c136..43ab3a8af500 100644 --- a/test/ng/directive/selectSpec.js +++ b/test/ng/directive/selectSpec.js @@ -1150,6 +1150,46 @@ describe('select', function() { }); }); + describe('disabled blank', function() { + it('should select disabled blank by default', function() { + var html = ''; + scope.$apply(function() { + scope.choices = ['A', 'B', 'C']; + }); + + compile(html); + + var options = element.find('option'); + var optionToSelect = options.eq(0); + expect(optionToSelect.text()).toBe('Choose One'); + expect(optionToSelect.prop('selected')).toBe(true); + expect(element[0].value).toBe(''); + + dealoc(element); + }); + + + it('should select disabled blank by default when select is required', function() { + var html = ''; + scope.$apply(function() { + scope.choices = ['A', 'B', 'C']; + }); + + compile(html); + + var options = element.find('option'); + var optionToSelect = options.eq(0); + expect(optionToSelect.text()).toBe('Choose One'); + expect(optionToSelect.prop('selected')).toBe(true); + expect(element[0].value).toBe(''); + + dealoc(element); + }); + }); describe('select-many', function() { From 9340e540259ce455bf29703b05029d8197d5dc99 Mon Sep 17 00:00:00 2001 From: Erin Altenhof-Long Date: Wed, 16 Jul 2014 15:11:52 -0700 Subject: [PATCH 3/5] test(select): add test of updating the model because of ng-change A regression #7855 was introduced by https://github.com/angular/angular.js/commit/dc149de9364c66b988f169f67cad39577ba43434 This test ensures that reverting that commit fixes this regression. In the regression, changes to a bound model in ng-change were not propagated back to the view. Test for #7855 --- test/ng/directive/selectSpec.js | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/test/ng/directive/selectSpec.js b/test/ng/directive/selectSpec.js index 43ab3a8af500..53718086ce1d 100644 --- a/test/ng/directive/selectSpec.js +++ b/test/ng/directive/selectSpec.js @@ -1148,6 +1148,31 @@ describe('select', function() { browserTrigger(element, 'change'); expect(scope.selected).toEqual(null); }); + + + // Regression https://github.com/angular/angular.js/issues/7855 + it('should update the model with ng-change', function() { + createSelect({ + 'ng-change':'change()', + 'ng-model':'selected', + 'ng-options':'value for value in values' + }); + + scope.$apply(function() { + scope.values = ['A', 'B']; + scope.selected = 'A'; + }); + + scope.change = function() { + scope.selected = 'A'; + }; + + element.find('option')[1].selected = true; + + browserTrigger(element, 'change'); + expect(element.find('option')[0].selected).toBeTruthy(); + expect(scope.selected).toEqual('A'); + }); }); describe('disabled blank', function() { @@ -1237,6 +1262,7 @@ describe('select', function() { expect(scope.selected).toEqual([scope.values[0]]); }); + it('should select from object', function() { createSelect({ 'ng-model':'selected', From cbfd9364a9ee0e60ce5668766eb56cb80f72086f Mon Sep 17 00:00:00 2001 From: Peter Bacon Darwin Date: Mon, 28 Jul 2014 09:18:54 +0100 Subject: [PATCH 4/5] test(select): add extra expectations and comments for clarity --- test/ng/directive/selectSpec.js | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/test/ng/directive/selectSpec.js b/test/ng/directive/selectSpec.js index 53718086ce1d..3261605ffbde 100644 --- a/test/ng/directive/selectSpec.js +++ b/test/ng/directive/selectSpec.js @@ -735,6 +735,8 @@ describe('select', function() { it('should not update selected property of an option element on digest with no change event', function() { + // ng-options="value.name for value in values" + // ng-model="selected" createSingleSelect(); scope.$apply(function() { @@ -743,6 +745,11 @@ describe('select', function() { }); var options = element.find('option'); + + expect(scope.selected).toEqual({ name: 'A' }); + expect(options.eq(0).prop('selected')).toBe(true); + expect(options.eq(1).prop('selected')).toBe(false); + var optionToSelect = options.eq(1); expect(optionToSelect.text()).toBe('B'); From 7164adc04e3d4766054687ee31816162336bc8ed Mon Sep 17 00:00:00 2001 From: Peter Bacon Darwin Date: Mon, 28 Jul 2014 09:23:25 +0100 Subject: [PATCH 5/5] fix(select): do not update selected property of an option element on digest with no change event The `render()` method was being invoked on every turn of the digest cycle, which was inadvertently updating the DOM even when a `change` event had not been triggered. This change only calls the `render()` method when `ctrl.$render()` is called, as part of the NgModelController` lifecycle and when the `modelValue` has significantly changed. --- src/ng/directive/select.js | 37 +++++++++++++++++++++++-------------- 1 file changed, 23 insertions(+), 14 deletions(-) diff --git a/src/ng/directive/select.js b/src/ng/directive/select.js index 03c65992f0a9..91585934445d 100644 --- a/src/ng/directive/select.js +++ b/src/ng/directive/select.js @@ -407,13 +407,33 @@ var selectDirective = ['$compile', '$parse', function($compile, $parse) { } } ctrl.$setViewValue(value); + render(); }); }); ctrl.$render = render; - // TODO(vojta): can't we optimize this ? - scope.$watch(render); + scope.$watch(valuesFn, render, true); + scope.$watch(getSelectedSet, render, true); + + function getSelectedSet() { + var selectedSet = false; + if (multiple) { + var modelValue = ctrl.$modelValue; + if (trackFn && isArray(modelValue)) { + selectedSet = new HashMap([]); + var locals = {}; + for (var trackIndex = 0; trackIndex < modelValue.length; trackIndex++) { + locals[valueName] = modelValue[trackIndex]; + selectedSet.put(trackFn(scope, locals), modelValue[trackIndex]); + } + } else { + selectedSet = new HashMap(modelValue); + } + } + return selectedSet; + } + function render() { // Temporary location for the option groups before we render them @@ -431,22 +451,11 @@ var selectDirective = ['$compile', '$parse', function($compile, $parse) { groupIndex, index, locals = {}, selected, - selectedSet = false, // nothing is selected yet + selectedSet = getSelectedSet(), lastElement, element, label; - if (multiple) { - if (trackFn && isArray(modelValue)) { - selectedSet = new HashMap([]); - for (var trackIndex = 0; trackIndex < modelValue.length; trackIndex++) { - locals[valueName] = modelValue[trackIndex]; - selectedSet.put(trackFn(scope, locals), modelValue[trackIndex]); - } - } else { - selectedSet = new HashMap(modelValue); - } - } // We now build up the list of options we need (we merge later) for (index = 0; length = keys.length, index < length; index++) {