From bfdaa24881151dcf1162432d30b3fd7dc39324d1 Mon Sep 17 00:00:00 2001 From: Erin Altenhof-Long Date: Wed, 16 Jul 2014 11:30:19 -0700 Subject: [PATCH 1/6] 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. Closes #7715 #7855 --- src/ng/directive/select.js | 8 +------- test/ng/directive/selectSpec.js | 21 --------------------- 2 files changed, 1 insertion(+), 28 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 diff --git a/test/ng/directive/selectSpec.js b/test/ng/directive/selectSpec.js index 9ce01704c136..0a70517a5b09 100644 --- a/test/ng/directive/selectSpec.js +++ b/test/ng/directive/selectSpec.js @@ -733,27 +733,6 @@ describe('select', function() { expect(sortedHtml(options[2])).toEqual(''); }); - it('should not update selected property of an option element on digest with no change event', - function() { - createSingleSelect(); - - scope.$apply(function() { - scope.values = [{name: 'A'}, {name: 'B'}, {name: 'C'}]; - scope.selected = scope.values[0]; - }); - - var options = element.find('option'); - var optionToSelect = options.eq(1); - - expect(optionToSelect.text()).toBe('B'); - - optionToSelect.prop('selected', true); - scope.$digest(); - - expect(optionToSelect.prop('selected')).toBe(true); - expect(scope.selected).toBe(scope.values[0]); - }); - describe('binding', function() { it('should bind to scope value', function() { From fb2e5a46d540be1b2753d95d870cd4d070c5a998 Mon Sep 17 00:00:00 2001 From: Erin Altenhof-Long Date: Mon, 28 Jul 2014 09:29:42 -0700 Subject: [PATCH 2/6] test(select): add test against updating selected property on digest with no change event Commit dc149de9364c66b988f169f67cad39577ba43434 was reverted to fix regressions #7715 and #7855. This commit introduced this test case and a corresponding fix for preventing the update of the selected property of an option element on a digest with no change event. Although the previous fix introduced regressions, the test covers a valid issue and should be included. --- test/ng/directive/selectSpec.js | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/test/ng/directive/selectSpec.js b/test/ng/directive/selectSpec.js index 0a70517a5b09..9ce01704c136 100644 --- a/test/ng/directive/selectSpec.js +++ b/test/ng/directive/selectSpec.js @@ -733,6 +733,27 @@ describe('select', function() { expect(sortedHtml(options[2])).toEqual(''); }); + it('should not update selected property of an option element on digest with no change event', + function() { + createSingleSelect(); + + scope.$apply(function() { + scope.values = [{name: 'A'}, {name: 'B'}, {name: 'C'}]; + scope.selected = scope.values[0]; + }); + + var options = element.find('option'); + var optionToSelect = options.eq(1); + + expect(optionToSelect.text()).toBe('B'); + + optionToSelect.prop('selected', true); + scope.$digest(); + + expect(optionToSelect.prop('selected')).toBe(true); + expect(scope.selected).toBe(scope.values[0]); + }); + describe('binding', function() { it('should bind to scope value', function() { From b7a2cc5ebd9a4564f9935491e9015e2d4437da07 Mon Sep 17 00:00:00 2001 From: Erin Altenhof-Long Date: Wed, 16 Jul 2014 11:43:47 -0700 Subject: [PATCH 3/6] 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 e4995f40adb2175523bb863b9826c8b9fb93ac27 Mon Sep 17 00:00:00 2001 From: Erin Altenhof-Long Date: Wed, 16 Jul 2014 15:11:52 -0700 Subject: [PATCH 4/6] 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 03b518c9c9e1950ae83a1cd3a45ba1d17d28bb32 Mon Sep 17 00:00:00 2001 From: Peter Bacon Darwin Date: Mon, 28 Jul 2014 09:18:54 +0100 Subject: [PATCH 5/6] 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 6140861ba89c5c9999c8e4e2fd74878299b107ea Mon Sep 17 00:00:00 2001 From: Peter Bacon Darwin Date: Mon, 28 Jul 2014 09:23:25 +0100 Subject: [PATCH 6/6] 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++) {