From 0a284b169577bcbcb459c69d30ab36644acb2ec4 Mon Sep 17 00:00:00 2001 From: Jeff Balboni Date: Sun, 26 Jan 2014 16:17:36 -0500 Subject: [PATCH 1/3] fix(select): avoid checking option element selected properties in render In Firefox, hovering over an option in an open select menu updates the selected property of option elements. This means that when a render is triggered by the digest cycle, and the list of options is being rendered, the selected properties are reset to the values from the model and the option hovered over changes. This fix changes the code to only use DOM elements' selected properties in a comparison when a change event has been fired. Otherwise, the internal new and existing option arrays are used. Closes #2448 --- src/ng/directive/select.js | 10 ++++++++-- test/ng/directive/selectSpec.js | 15 +++++++++++++++ 2 files changed, 23 insertions(+), 2 deletions(-) diff --git a/src/ng/directive/select.js b/src/ng/directive/select.js index e44b61e955cb..2222a392e607 100644 --- a/src/ng/directive/select.js +++ b/src/ng/directive/select.js @@ -322,7 +322,8 @@ var selectDirective = ['$compile', '$parse', function($compile, $parse) { // We try to reuse these if possible // - optionGroupsCache[0] is the options with no option group // - optionGroupsCache[?][0] is the parent: either the SELECT or OPTGROUP element - optionGroupsCache = [[{element: selectElement, label:''}]]; + optionGroupsCache = [[{element: selectElement, label:''}]], + changeEventFired = false; if (nullOption) { // compile the element since there might be bindings in it @@ -341,6 +342,7 @@ var selectDirective = ['$compile', '$parse', function($compile, $parse) { selectElement.empty(); selectElement.on('change', function() { + changeEventFired = true; scope.$apply(function() { var optionGroup, collection = valuesFn(scope) || [], @@ -529,7 +531,10 @@ var selectDirective = ['$compile', '$parse', function($compile, $parse) { lastElement.val(existingOption.id = option.id); } // lastElement.prop('selected') provided by jQuery has side-effects - if (lastElement[0].selected !== option.selected) { + // FF will update selected property on DOM elements when hovering, + // so we shouldn't check those unless a change event has happened + if ((changeEventFired && lastElement[0].selected !== option.selected) || + existingOption.selected !== option.selected) { lastElement.prop('selected', (existingOption.selected = option.selected)); } } else { @@ -573,6 +578,7 @@ var selectDirective = ['$compile', '$parse', function($compile, $parse) { while(optionGroupsCache.length > groupIndex) { optionGroupsCache.pop()[0].element.remove(); } + changeEventFired = false; } } } diff --git a/test/ng/directive/selectSpec.js b/test/ng/directive/selectSpec.js index 6fcd1fe05f82..d2215506d2f1 100644 --- a/test/ng/directive/selectSpec.js +++ b/test/ng/directive/selectSpec.js @@ -733,6 +733,21 @@ describe('select', function() { expect(sortedHtml(options[2])).toEqual(''); }); + it('should ignore option object selected changes', 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); + optionToSelect.prop('selected', true); + scope.$digest(); + expect(optionToSelect.prop('selected')).toBe(true); + }); + describe('binding', function() { it('should bind to scope value', function() { From 956f65dec2c491a348f5cf1eeba2450945551310 Mon Sep 17 00:00:00 2001 From: Jeff Balboni Date: Wed, 29 Jan 2014 23:42:17 -0500 Subject: [PATCH 2/3] Clean up null option explicitly, instead of tracking change event --- src/ng/directive/select.js | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/ng/directive/select.js b/src/ng/directive/select.js index 2222a392e607..08d75174371a 100644 --- a/src/ng/directive/select.js +++ b/src/ng/directive/select.js @@ -322,8 +322,7 @@ var selectDirective = ['$compile', '$parse', function($compile, $parse) { // We try to reuse these if possible // - optionGroupsCache[0] is the options with no option group // - optionGroupsCache[?][0] is the parent: either the SELECT or OPTGROUP element - optionGroupsCache = [[{element: selectElement, label:''}]], - changeEventFired = false; + optionGroupsCache = [[{element: selectElement, label:''}]]; if (nullOption) { // compile the element since there might be bindings in it @@ -342,7 +341,6 @@ var selectDirective = ['$compile', '$parse', function($compile, $parse) { selectElement.empty(); selectElement.on('change', function() { - changeEventFired = true; scope.$apply(function() { var optionGroup, collection = valuesFn(scope) || [], @@ -394,6 +392,12 @@ 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); }); @@ -531,10 +535,7 @@ var selectDirective = ['$compile', '$parse', function($compile, $parse) { lastElement.val(existingOption.id = option.id); } // lastElement.prop('selected') provided by jQuery has side-effects - // FF will update selected property on DOM elements when hovering, - // so we shouldn't check those unless a change event has happened - if ((changeEventFired && lastElement[0].selected !== option.selected) || - existingOption.selected !== option.selected) { + if (existingOption.selected !== option.selected) { lastElement.prop('selected', (existingOption.selected = option.selected)); } } else { @@ -578,7 +579,6 @@ var selectDirective = ['$compile', '$parse', function($compile, $parse) { while(optionGroupsCache.length > groupIndex) { optionGroupsCache.pop()[0].element.remove(); } - changeEventFired = false; } } } From ef9b16ddd5c21b3fa5f3f5adda3ed0be6a70c617 Mon Sep 17 00:00:00 2001 From: Jeff Balboni Date: Fri, 31 Jan 2014 08:40:25 -0500 Subject: [PATCH 3/3] Updated test description and added expect statements --- test/ng/directive/selectSpec.js | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/test/ng/directive/selectSpec.js b/test/ng/directive/selectSpec.js index d2215506d2f1..c251dbc37526 100644 --- a/test/ng/directive/selectSpec.js +++ b/test/ng/directive/selectSpec.js @@ -733,7 +733,8 @@ describe('select', function() { expect(sortedHtml(options[2])).toEqual(''); }); - it('should ignore option object selected changes', function() { + it('should not update selected property of an option element on digest with no change event', + function() { createSingleSelect(); scope.$apply(function() { @@ -743,9 +744,14 @@ describe('select', function() { 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() {