From f02b707b5e4a5ffd1e1a20d910754cfabfc19622 Mon Sep 17 00:00:00 2001 From: Martin Staffa Date: Sat, 6 Feb 2016 02:08:28 +0100 Subject: [PATCH 1/4] feat(select): support values of any type added with ngValue select elements with ngModel will now set ngModel to option values added by ngValue. This allows setting values of any type (not only strings) without the use of ngOptions. Interpolations inside attributes can only be strings, but the ngValue directive uses attrs.$set, which does not have any type restriction. Any $observe on the value attribute will therefore receive the original value (result of ngValue expression). However, when a user selects an option, the browser sets the select value to the actual option's value attribute, which is still always a string. For that reason, when options are added by ngValue, we set the hashed value of the original value in the value attribute and store the actual value in an extra map. When the select value changes, we read access the actual value via the hashed select value. Since we only use a hashed value for ngValue, we will have extra checks for the hashed values: - when options are read, for both single and multiple select - when options are written, for multiple select I don't expect this to have a performance impact, but it should be kept in mind. Closes #9842 Closes #6297 BREAKING CHANGE: `' }; }); + + $compileProvider.directive('exposeAttributes', function() { + return { + require: '^^select', + link: { + pre: function(scope, element, attrs, ctrl) { + optionAttributesList.push(attrs); + } + } + }; + }); + })); beforeEach(inject(function($rootScope, _$compile_) { @@ -297,7 +309,7 @@ describe('select', function() { expect(selectCtrl.writeValue).not.toHaveBeenCalled(); scope.$digest(); - expect(selectCtrl.writeValue).toHaveBeenCalledOnce(); + expect(selectCtrl.writeValue).toHaveBeenCalled(); dealoc(select); }); @@ -1224,5 +1236,224 @@ describe('select', function() { }).toThrowMinErr('ng','badname', 'hasOwnProperty is not a valid "option value" name'); }); + describe('with ngValue (and non-primitive values)', function() { + + they('should set the option attribute and select it for value $prop', [ + 'string', + undefined, + 1, + true, + null, + {prop: 'value'}, + ['a'], + NaN + ], function(prop) { + scope.option1 = prop; + scope.selected = 'NOMATCH'; + + compile(''); + + scope.$digest(); + expect(element.find('option').eq(0).val()).toBe('? string:NOMATCH ?'); + + scope.selected = prop; + scope.$digest(); + + expect(element.find('option').eq(0).val()).toBe(hashKey(prop)); + + // Reset + scope.selected = false; + scope.$digest(); + + expect(element.find('option').eq(0).val()).toBe('? boolean:false ?'); + + browserTrigger(element.find('option').eq(0)); + if (typeof prop === 'number' && isNaN(prop)) { + expect(scope.selected).toBeNaN(); + } else { + expect(scope.selected).toBe(prop); + } + }); + + + they('should update the option attribute and select it for value $prop', [ + 'string', + undefined, + 1, + true, + null, + {prop: 'value'}, + ['a'], + NaN + ], function(prop) { + scope.option = prop; + scope.selected = 'NOMATCH'; + + compile(''); + + var selectController = element.controller('select'); + spyOn(selectController, 'removeOption').and.callThrough(); + + scope.$digest(); + expect(selectController.removeOption).not.toHaveBeenCalled(); + expect(element.find('option').eq(0).val()).toBe('? string:NOMATCH ?'); + + scope.selected = prop; + scope.$digest(); + + expect(element.find('option').eq(0).val()).toBe(hashKey(prop)); + expect(element[0].selectedIndex).toBe(0); + + scope.option = 'UPDATEDVALUE'; + scope.$digest(); + + expect(selectController.removeOption.calls.count()).toBe(1); + + // Updating the option value currently does not update the select model + if (typeof prop === 'number' && isNaN(prop)) { + expect(selectController.removeOption.calls.argsFor(0)[0]).toBeNaN(); + } else { + expect(selectController.removeOption.calls.argsFor(0)[0]).toBe(prop); + } + + expect(scope.selected).toBe(null); + expect(element[0].selectedIndex).toBe(0); + expect(element.find('option').length).toBe(2); + expect(element.find('option').eq(0).prop('selected')).toBe(true); + expect(element.find('option').eq(0).val()).toBe(unknownValue(prop)); + expect(element.find('option').eq(1).prop('selected')).toBe(false); + expect(element.find('option').eq(1).val()).toBe('string:UPDATEDVALUE'); + + scope.selected = 'UPDATEDVALUE'; + scope.$digest(); + + expect(element[0].selectedIndex).toBe(0); + expect(element.find('option').eq(0).val()).toBe('string:UPDATEDVALUE'); + }); + + it('should interact with custom attribute $observe and $set calls', function() { + var log = [], optionAttr; + + compile(''); + + optionAttr = optionAttributesList[0]; + optionAttr.$observe('value', function(newVal) { + log.push(newVal); + }); + + scope.option = 'init'; + scope.$digest(); + + expect(log[0]).toBe('init'); + expect(element.find('option').eq(1).val()).toBe('string:init'); + + optionAttr.$set('value', 'update'); + expect(log[1]).toBe('update'); + expect(element.find('option').eq(1).val()).toBe('string:update'); + + }); + + it('should ignore the option text / value attribute if the ngValue attribute exists', function() { + scope.ngvalue = 'abc'; + scope.value = 'def'; + scope.textvalue = 'ghi'; + + compile(''); + expect(element).toEqualSelect([unknownValue(undefined)], 'string:abc'); + }); + + it('should ignore option text with multiple interpolations if the ngValue attribute exists', function() { + scope.ngvalue = 'abc'; + scope.textvalue = 'def'; + scope.textvalue2 = 'ghi'; + + compile(''); + expect(element).toEqualSelect([unknownValue(undefined)], 'string:abc'); + }); + + describe('and select[multiple]', function() { + + it('should allow multiple selection', function() { + scope.options = { + a: 'string', + b: undefined, + c: 1, + d: true, + e: null, + f: {prop: 'value'}, + g: ['a'], + h: NaN + }; + scope.selected = []; + + compile(''); + + scope.$digest(); + expect(element).toEqualSelect( + 'string:string', + 'undefined:undefined', + 'number:1', + 'boolean:true', + 'object:null', + 'object:4', + 'object:5', + 'number:NaN' + ); + + scope.selected = ['string', 1]; + scope.$digest(); + + expect(element.find('option').eq(0).prop('selected')).toBe(true); + expect(element.find('option').eq(2).prop('selected')).toBe(true); + + browserTrigger(element.find('option').eq(1)); + expect(scope.selected).toEqual([undefined]); + + //reset + scope.selected = []; + scope.$digest(); + + forEach(element.find('option'), function(option) { + // browserTrigger can't produce click + ctrl, so set selection manually + jqLite(option).prop('selected', true); + }); + + browserTrigger(element, 'change'); + + var arrayVal = ['a']; + arrayVal.$$hashKey = 'object:5'; + + expect(scope.selected).toEqual([ + 'string', + undefined, + 1, + true, + null, + {prop: 'value', $$hashKey: 'object:4'}, + arrayVal, + NaN + ]); + }); + + }); + + + }); + }); }); From 47c15fbcc10f118170813021e8e605ffd263ad84 Mon Sep 17 00:00:00 2001 From: Martin Staffa Date: Sat, 21 May 2016 13:47:24 +0200 Subject: [PATCH 2/4] fix(select): handle model updates when options are manipulated These rules follow ngOptions behavior: - when an option that is currently selected, is removed or its value changes, the model is set to null. - when an an option is added or its value changes to match the currently selected model, this option is selected. - when an option is disabled, the model is set to null. - when the model value changes to a value that matches a disabled option, this option is selected (analogue to ngOptions) --- src/ng/directive/select.js | 64 ++- test/ng/directive/selectSpec.js | 748 +++++++++++++++++++++++++++++++- 2 files changed, 804 insertions(+), 8 deletions(-) diff --git a/src/ng/directive/select.js b/src/ng/directive/select.js index 92617667c506..c99b6fc428c2 100644 --- a/src/ng/directive/select.js +++ b/src/ng/directive/select.js @@ -20,6 +20,7 @@ var SelectController = // If the ngModel doesn't get provided then provide a dummy noop version to prevent errors self.ngModelCtrl = noopNgModelController; + self.multiple = false; // The "unknown" option is one that is prepended to the list if the viewValue // does not match any of the options. When it is rendered the value of the unknown @@ -91,7 +92,9 @@ var SelectController = } var count = optionsMap.get(value) || 0; optionsMap.put(value, count + 1); - self.ngModelCtrl.$render(); + // Only render at the end of a digest. This improves render performance when many options + // are added during a digest and ensures all relevant options are correctly marked as selected + scheduleRender(); }; // Tell the select control that an option, with the given value, has been removed @@ -115,6 +118,15 @@ var SelectController = }; + var renderScheduled = false; + function scheduleRender() { + if (renderScheduled) return; + renderScheduled = true; + $scope.$$postDigest(function() { + renderScheduled = false; + self.ngModelCtrl.$render(); + }); + } var updateScheduled = false; function scheduleViewValueUpdate(renderAfter) { @@ -163,29 +175,74 @@ var SelectController = } else if (interpolateValueFn) { // The value attribute is interpolated optionAttrs.$observe('value', function valueAttributeObserveAction(newVal) { + var currentVal = self.readValue(); + var removal; + var previouslySelected = optionElement.prop('selected'); + var removedVal; + if (isDefined(oldVal)) { self.removeOption(oldVal); + removal = true; + removedVal = oldVal; } oldVal = newVal; self.addOption(newVal, optionElement); + + if (removal && previouslySelected) { + scheduleViewValueUpdate(); + } }); } else if (interpolateTextFn) { // The text content is interpolated optionScope.$watch(interpolateTextFn, function interpolateWatchAction(newVal, oldVal) { optionAttrs.$set('value', newVal); + var previouslySelected = optionElement.prop('selected'); if (oldVal !== newVal) { self.removeOption(oldVal); } self.addOption(newVal, optionElement); + + if (oldVal && previouslySelected) { + scheduleViewValueUpdate(); + } }); } else { // The value attribute is static self.addOption(optionAttrs.value, optionElement); } + + var oldDisabled; + optionAttrs.$observe('disabled', function(newVal) { + + // Since model updates will also select disabled options (like ngOptions), + // we only have to handle options becoming disabled, not enabled + + if (newVal === 'true' || newVal && optionElement.prop('selected')) { + if (self.multiple) { + scheduleViewValueUpdate(true); + } else { + self.ngModelCtrl.$setViewValue(null); + self.ngModelCtrl.$render(); + } + oldDisabled = newVal; + } + }); + optionElement.on('$destroy', function() { - self.removeOption(optionAttrs.value); + var currentValue = self.readValue(); + var removeValue = optionAttrs.value; + + self.removeOption(removeValue); self.ngModelCtrl.$render(); + + if (self.multiple && currentValue && currentValue.indexOf(removeValue) !== -1 || + currentValue === removeValue + ) { + // When multiple (selected) options are destroyed at the same time, we don't want + // to run a model update for each of them. Instead, run a single update in the $$postDigest + scheduleViewValueUpdate(true); + } }); }; }]; @@ -477,12 +534,13 @@ var selectDirective = function() { // we have to add an extra watch since ngModel doesn't work well with arrays - it // doesn't trigger rendering if only an item in the array changes. if (attr.multiple) { + selectCtrl.multiple = true; // Read value now needs to check each option to see if it is selected selectCtrl.readValue = function readMultipleValue() { var array = []; forEach(element.find('option'), function(option) { - if (option.selected) { + if (option.selected && !option.disabled) { var val = option.value; array.push(val in selectCtrl.selectValueMap ? selectCtrl.selectValueMap[val] : val); } diff --git a/test/ng/directive/selectSpec.js b/test/ng/directive/selectSpec.js index 70d3b2d53f15..9027aa657326 100644 --- a/test/ng/directive/selectSpec.js +++ b/test/ng/directive/selectSpec.js @@ -629,20 +629,27 @@ describe('select', function() { scope.$apply(function() { scope.robots.pop(); }); - expect(element).toEqualSelect([unknownValue('r2d2')], 'c3p0'); - expect(scope.robot).toBe('r2d2'); + expect(element).toEqualSelect([unknownValue(null)], 'c3p0'); + expect(scope.robot).toBe(null); scope.$apply(function() { scope.robots.unshift('r2d2'); }); + expect(element).toEqualSelect([unknownValue(null)], 'r2d2', 'c3p0'); + expect(scope.robot).toBe(null); + + scope.$apply(function() { + scope.robot = 'r2d2'; + }); + expect(element).toEqualSelect(['r2d2'], 'c3p0'); - expect(scope.robot).toBe('r2d2'); scope.$apply(function() { delete scope.robots; }); - expect(element).toEqualSelect([unknownValue('r2d2')]); - expect(scope.robot).toBe('r2d2'); + + expect(element).toEqualSelect([unknownValue(null)]); + expect(scope.robot).toBe(null); }); }); @@ -1452,8 +1459,739 @@ describe('select', function() { }); + }); + + describe('updating the model and selection when option elements are manipulated', function() { + + they('should set the model to null when the currently selected option with $prop is removed', + ['ngValue', 'interpolatedValue', 'interpolatedText'], function(prop) { + + var A = { name: 'A'}, B = { name: 'B'}, C = { name: 'C'}; + + scope.options = [A, B, C]; + scope.obj = {}; + + var optionString = ''; + + switch (prop) { + case 'ngValue': + optionString = ''; + break; + case 'interpolatedValue': + optionString = ''; + break; + case 'interpolatedText': + optionString = ''; + break; + } + + compile( + '' + ); + + var optionElements = element.find('option'); + expect(optionElements.length).toEqual(4); + browserTrigger(optionElements.eq(0)); + + optionElements = element.find('option'); + expect(optionElements.length).toEqual(3); + expect(scope.obj.value).toBe(prop === 'ngValue' ? A : 'A'); + + scope.options.shift(); + scope.$digest(); + + optionElements = element.find('option'); + expect(optionElements.length).toEqual(3); + expect(scope.obj.value).toBe(null); + expect(element.val()).toBe('? object:null ?'); + }); + + + they('should set the model to null when the currently selected option with $prop changes its value', + [ + 'ngValue', + 'interpolatedValue', + 'interpolatedText' + ], function(prop) { + + var A = { name: 'A'}, B = { name: 'B'}, C = { name: 'C'}; + + scope.options = [A, B, C]; + scope.obj = {}; + + var optionString = ''; + + switch (prop) { + case 'ngValue': + optionString = ''; + break; + case 'interpolatedValue': + optionString = ''; + break; + case 'interpolatedText': + optionString = ''; + break; + } + + compile( + '' + ); + + var optionElements = element.find('option'); + expect(optionElements.length).toEqual(4); + browserTrigger(optionElements.eq(0)); + + optionElements = element.find('option'); + expect(optionElements.length).toEqual(3); + expect(scope.obj.value).toBe('A'); + + A.name = 'X'; + scope.$digest(); + + optionElements = element.find('option'); + expect(optionElements.length).toEqual(4); + expect(scope.obj.value).toBe(null); + expect(element.val()).toBe('? string:A ?'); + }); + + + they('should set the model to null when the currently selected option with $prop is disabled', + [ + 'ngValue', + 'interpolatedValue', + 'interpolatedText' + ], function(prop) { + + var A = { name: 'A'}, B = { name: 'B'}, C = { name: 'C'}; + + scope.options = [A, B, C]; + scope.obj = {}; + + var optionString = ''; + + switch (prop) { + case 'ngValue': + optionString = ''; + break; + case 'interpolatedValue': + optionString = ''; + break; + case 'interpolatedText': + optionString = ''; + break; + } + + compile( + '' + ); + + var optionElements = element.find('option'); + expect(optionElements.length).toEqual(4); + browserTrigger(optionElements.eq(0)); + + optionElements = element.find('option'); + expect(optionElements.length).toEqual(3); + expect(scope.obj.value).toBe('A'); + + A.disabled = true; + scope.$digest(); + + optionElements = element.find('option'); + expect(optionElements.length).toEqual(4); + expect(scope.obj.value).toBe(null); + expect(element.val()).toBe('? object:null ?'); + }); + + + they('should select a disabled option with $prop when the model is set to the matching value', + [ + 'ngValue', + 'interpolatedValue', + 'interpolatedText' + ], function(prop) { + + var A = { name: 'A'}, B = { name: 'B'}, C = { name: 'C'}; + + scope.options = [A, B, C]; + scope.obj = {}; + + var optionString = ''; + + switch (prop) { + case 'ngValue': + optionString = ''; + break; + case 'interpolatedValue': + optionString = ''; + break; + case 'interpolatedText': + optionString = ''; + break; + } + + compile( + '' + ); + + var optionElements = element.find('option'); + expect(optionElements.length).toEqual(4); + expect(optionElements[0].value).toEqual(unknownValue(undefined)); + + B.disabled = true; + scope.$digest(); + + optionElements = element.find('option'); + expect(optionElements.length).toEqual(4); + expect(optionElements[0].value).toEqual(unknownValue(undefined)); + + scope.obj.value = 'B'; + scope.$digest(); + + optionElements = element.find('option'); + expect(optionElements.length).toEqual(3); + expect(scope.obj.value).toBe('B'); + // jQuery returns null for val() when the option is disabled, see + // https://bugs.jquery.com/ticket/13097 + expect(element[0].value).toBe(prop === 'ngValue' ? 'string:B' : 'B'); + expect(optionElements.eq(1).prop('selected')).toBe(true); + }); + + + they('should ignore an option with $prop that becomes enabled and does not match the model', + [ + 'ngValue', + 'interpolatedValue', + 'interpolatedText' + ], function(prop) { + + var A = { name: 'A'}, B = { name: 'B'}, C = { name: 'C'}; + + scope.options = [A, B, C]; + scope.obj = {}; + + var optionString = ''; + + switch (prop) { + case 'ngValue': + optionString = ''; + break; + case 'interpolatedValue': + optionString = ''; + break; + case 'interpolatedText': + optionString = ''; + break; + } + + compile( + '' + ); + + var optionElements = element.find('option'); + expect(optionElements.length).toEqual(4); + browserTrigger(optionElements.eq(0)); + + optionElements = element.find('option'); + expect(optionElements.length).toEqual(3); + expect(scope.obj.value).toBe('A'); + + A.disabled = true; + scope.$digest(); + + optionElements = element.find('option'); + expect(optionElements.length).toEqual(4); + expect(scope.obj.value).toBe(null); + expect(element.val()).toBe('? object:null ?'); + + A.disabled = false; + scope.$digest(); + + optionElements = element.find('option'); + expect(optionElements.length).toEqual(4); + expect(scope.obj.value).toBe(null); + expect(element.val()).toBe('? object:null ?'); + }); + + + they('should select a newly added option with $prop when it matches the current model', + [ + 'ngValue', + 'interpolatedValue', + 'interpolatedText' + ], function(prop) { + + var A = { name: 'A'}, B = { name: 'B'}, C = { name: 'C'}; + + scope.options = [A, B]; + scope.obj = { + value: prop === 'ngValue' ? C : 'C' + }; + + var optionString = ''; + + switch (prop) { + case 'ngValue': + optionString = ''; + break; + case 'interpolatedValue': + optionString = ''; + break; + case 'interpolatedText': + optionString = ''; + break; + } + + compile( + '' + ); + + var optionElements = element.find('option'); + expect(optionElements.length).toEqual(3); + + scope.options.push(C); + scope.$digest(); + + optionElements = element.find('option'); + expect(element.val()).toBe(prop === 'ngValue' ? 'object:4' : 'C'); + expect(optionElements.length).toEqual(3); + expect(optionElements[2].selected).toBe(true); + expect(scope.obj.value).toEqual(prop === 'ngValue' ? {name: 'C', $$hashKey: 'object:4'} : 'C'); + }); + + + they('should keep selection and model when repeated options with track by are replaced with equal options', + [ + 'ngValue', + 'interpolatedValue', + 'interpolatedText' + ], function(prop) { + + var A = { name: 'A'}, B = { name: 'B'}, C = { name: 'C'}; + + scope.options = [A, B, C]; + scope.obj = { + value: 'C' + }; + + var optionString = ''; + + switch (prop) { + case 'ngValue': + optionString = ''; + break; + case 'interpolatedValue': + optionString = ''; + break; + case 'interpolatedText': + optionString = ''; + break; + } + + compile( + '' + ); + + var optionElements = element.find('option'); + expect(optionElements.length).toEqual(3); + + scope.obj.value = 'C'; + scope.$digest(); + + optionElements = element.find('option'); + expect(element.val()).toBe(prop === 'ngValue' ? 'string:C' : 'C'); + expect(optionElements.length).toEqual(3); + expect(optionElements[2].selected).toBe(true); + expect(scope.obj.value).toBe('C'); + + scope.options = [ + {name: 'A'}, + {name: 'B'}, + {name: 'C'} + ]; + scope.$digest(); + + optionElements = element.find('option'); + expect(element.val()).toBe(prop === 'ngValue' ? 'string:C' : 'C'); + expect(optionElements.length).toEqual(3); + expect(optionElements[2].selected).toBe(true); + expect(scope.obj.value).toBe('C'); + }); + + describe('when multiple', function() { + + they('should set the model to null when the currently selected option with $prop is removed', + [ + 'ngValue', + 'interpolatedValue', + 'interpolatedText' + ], function(prop) { + + var A = { name: 'A'}, B = { name: 'B'}, C = { name: 'C'}; + + scope.options = [A, B, C]; + scope.obj = {}; + + var optionString = ''; + + switch (prop) { + case 'ngValue': + optionString = ''; + break; + case 'interpolatedValue': + optionString = ''; + break; + case 'interpolatedText': + optionString = ''; + break; + } + + compile( + '' + ); + + var ngModelCtrl = element.controller('ngModel'); + var ngModelCtrlSpy = spyOn(ngModelCtrl, '$setViewValue').and.callThrough(); + + var optionElements = element.find('option'); + expect(optionElements.length).toEqual(3); + + optionElements.eq(0).prop('selected', true); + optionElements.eq(2).prop('selected', true); + browserTrigger(element); + + optionElements = element.find('option'); + expect(optionElements.length).toEqual(3); + expect(scope.obj.value).toEqual(prop === 'ngValue' ? [A, C] : ['A', 'C']); + + + ngModelCtrlSpy.calls.reset(); + scope.options.shift(); + scope.options.pop(); + scope.$digest(); + + optionElements = element.find('option'); + expect(optionElements.length).toEqual(1); + expect(scope.obj.value).toEqual([]); + expect(element.val()).toBe(null); + expect(ngModelCtrlSpy).toHaveBeenCalledTimes(1); + }); + + they('should set the model to null when the currently selected option with $prop changes its value', + [ + 'ngValue', + 'interpolatedValue', + 'interpolatedText' + ], function(prop) { + + var A = { name: 'A'}, B = { name: 'B'}, C = { name: 'C'}; + + scope.options = [A, B, C]; + scope.obj = {}; + + var optionString = ''; + + switch (prop) { + case 'ngValue': + optionString = ''; + break; + case 'interpolatedValue': + optionString = ''; + break; + case 'interpolatedText': + optionString = ''; + break; + } + + compile( + '' + ); + + var ngModelCtrl = element.controller('ngModel'); + var ngModelCtrlSpy = spyOn(ngModelCtrl, '$setViewValue').and.callThrough(); + + var optionElements = element.find('option'); + expect(optionElements.length).toEqual(3); + + optionElements.eq(0).prop('selected', true); + optionElements.eq(2).prop('selected', true); + browserTrigger(element); + + optionElements = element.find('option'); + expect(optionElements.length).toEqual(3); + expect(scope.obj.value).toEqual(['A', 'C']); + + ngModelCtrlSpy.calls.reset(); + A.name = 'X'; + C.name = 'Z'; + scope.$digest(); + + optionElements = element.find('option'); + expect(optionElements.length).toEqual(3); + expect(scope.obj.value).toEqual([]); + expect(element.val()).toBe(null); + expect(ngModelCtrlSpy).toHaveBeenCalledTimes(1); + + }); + + they('should set the model to null when the currently selected option with $prop becomes disabled', + [ + 'ngValue', + 'interpolatedValue', + 'interpolatedText' + ], function(prop) { + + var A = { name: 'A'}, B = { name: 'B'}, C = { name: 'C'}, D = { name: 'D'}; + + scope.options = [A, B, C, D]; + scope.obj = {}; + + var optionString = ''; + + switch (prop) { + case 'ngValue': + optionString = ''; + break; + case 'interpolatedValue': + optionString = ''; + break; + case 'interpolatedText': + optionString = ''; + break; + } + + compile( + '' + ); + + var ngModelCtrl = element.controller('ngModel'); + var ngModelCtrlSpy = spyOn(ngModelCtrl, '$setViewValue').and.callThrough(); + + var optionElements = element.find('option'); + expect(optionElements.length).toEqual(4); + + optionElements.eq(0).prop('selected', true); + optionElements.eq(2).prop('selected', true); + optionElements.eq(3).prop('selected', true); + browserTrigger(element); + + optionElements = element.find('option'); + expect(optionElements.length).toEqual(4); + expect(scope.obj.value).toEqual(['A', 'C', 'D']); + + ngModelCtrlSpy.calls.reset(); + A.disabled = true; + C.disabled = true; + scope.$digest(); + + optionElements = element.find('option'); + expect(optionElements.length).toEqual(4); + expect(scope.obj.value).toEqual(['D']); + expect(element.val()).toEqual(prop === 'ngValue' ? ['string:D'] : ['D']); + expect(ngModelCtrlSpy).toHaveBeenCalledTimes(1); + }); + + + they('should select disabled options with $prop when the model is set to matching values', + [ + 'ngValue', + 'interpolatedValue', + 'interpolatedText' + ], function(prop) { + + var A = { name: 'A'}, B = { name: 'B'}, C = { name: 'C'}, D = {name: 'D'}; + + scope.options = [A, B, C, D]; + scope.obj = {}; + + var optionString = ''; + + switch (prop) { + case 'ngValue': + optionString = ''; + break; + case 'interpolatedValue': + optionString = ''; + break; + case 'interpolatedText': + optionString = ''; + break; + } + + compile( + '' + ); + + var optionElements = element.find('option'); + expect(optionElements.length).toEqual(4); + expect(element[0].value).toBe(''); + + A.disabled = true; + D.disabled = true; + scope.$digest(); + + optionElements = element.find('option'); + expect(optionElements.length).toEqual(4); + expect(element[0].value).toBe(''); + + scope.obj.value = prop === 'ngValue' ? [A, C, D] : ['A', 'C', 'D']; + scope.$digest(); + + optionElements = element.find('option'); + expect(optionElements.length).toEqual(4); + expect(scope.obj.value).toEqual(prop === 'ngValue' ? + [ + {name: 'A', $$hashKey: 'object:4', disabled: true}, + {name: 'C', $$hashKey: 'object:6'}, + {name: 'D', $$hashKey: 'object:7', disabled: true} + ] : + ['A', 'C', 'D'] + ); + + expect(optionElements.eq(0).prop('selected')).toBe(true); + expect(optionElements.eq(2).prop('selected')).toBe(true); + expect(optionElements.eq(3).prop('selected')).toBe(true); + }); + + they('should select a newly added option with $prop when it matches the current model', + [ + 'ngValue', + 'interpolatedValue', + 'interpolatedText' + ], function(prop) { + + var A = { name: 'A'}, B = { name: 'B'}, C = { name: 'C'}; + + scope.options = [A, B]; + scope.obj = { + value: prop === 'ngValue' ? [B, C] : ['B', 'C'] + }; + + var optionString = ''; + + switch (prop) { + case 'ngValue': + optionString = ''; + break; + case 'interpolatedValue': + optionString = ''; + break; + case 'interpolatedText': + optionString = ''; + break; + } + + compile( + '' + ); + + var optionElements = element.find('option'); + expect(optionElements.length).toEqual(2); + expect(optionElements.eq(1).prop('selected')).toBe(true); + + scope.options.push(C); + scope.$digest(); + + optionElements = element.find('option'); + expect(element.val()).toEqual(prop === 'ngValue' ? ['object:4', 'object:5'] : ['B', 'C']); + expect(optionElements.length).toEqual(3); + expect(optionElements[1].selected).toBe(true); + expect(optionElements[2].selected).toBe(true); + expect(scope.obj.value).toEqual(prop === 'ngValue' ? + [{ name: 'B', $$hashKey: 'object:4'}, + {name: 'C', $$hashKey: 'object:5'}] : + ['B', 'C']); + }); + + they('should keep selection and model when a repeated options with track by are replaced with equal options', + [ + 'ngValue', + 'interpolatedValue', + 'interpolatedText' + ], function(prop) { + + var A = { name: 'A'}, B = { name: 'B'}, C = { name: 'C'}; + + scope.options = [A, B, C]; + scope.obj = { + value: 'C' + }; + + var optionString = ''; + + switch (prop) { + case 'ngValue': + optionString = ''; + break; + case 'interpolatedValue': + optionString = ''; + break; + case 'interpolatedText': + optionString = ''; + break; + } + + compile( + '' + ); + + var optionElements = element.find('option'); + expect(optionElements.length).toEqual(3); + + scope.obj.value = ['B', 'C']; + scope.$digest(); + + optionElements = element.find('option'); + expect(element.val()).toEqual(prop === 'ngValue' ? ['string:B', 'string:C'] : ['B', 'C']); + expect(optionElements.length).toEqual(3); + expect(optionElements[1].selected).toBe(true); + expect(optionElements[2].selected).toBe(true); + expect(scope.obj.value).toEqual(['B', 'C']); + + scope.options = [ + {name: 'A'}, + {name: 'B'}, + {name: 'C'} + ]; + scope.$digest(); + + optionElements = element.find('option'); + expect(element.val()).toEqual(prop === 'ngValue' ? ['string:B', 'string:C'] : ['B', 'C']); + expect(optionElements.length).toEqual(3); + expect(optionElements[1].selected).toBe(true); + expect(optionElements[2].selected).toBe(true); + expect(scope.obj.value).toEqual(['B', 'C']); + }); + + }); }); + }); }); From ba36bde6736f0810ca670e10952a8e1c021de531 Mon Sep 17 00:00:00 2001 From: Martin Staffa Date: Wed, 1 Jun 2016 15:26:27 +0200 Subject: [PATCH 3/4] perf(select): don't prepend unknown option if already prepended --- src/ng/directive/select.js | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/ng/directive/select.js b/src/ng/directive/select.js index c99b6fc428c2..b583546d653f 100644 --- a/src/ng/directive/select.js +++ b/src/ng/directive/select.js @@ -36,6 +36,12 @@ var SelectController = $element.val(unknownVal); }; + self.updateUnknownOption = function(val) { + var unknownVal = '? ' + hashKey(val) + ' ?'; + self.unknownOption.val(unknownVal); + $element.val(unknownVal); + }; + $scope.$on('$destroy', function() { // disable unknown option so that we don't do work when the whole select is being destroyed self.renderUnknownOption = noop; @@ -74,6 +80,8 @@ var SelectController = if (value == null && self.emptyOption) { self.removeUnknownOption(); $element.val(''); + } else if (self.unknownOption.parent().length) { + self.updateUnknownOption(value); } else { self.renderUnknownOption(value); } From 7b50f498eb06f7249c83987fcca87f5297347c10 Mon Sep 17 00:00:00 2001 From: Peter Bacon Darwin Date: Mon, 30 May 2016 16:47:53 +0100 Subject: [PATCH 4/4] chore(benchpress): add benchmarks for select with ngValue --- benchmarks/select-ng-value-bp/app.js | 104 +++++++++++++++++++++++ benchmarks/select-ng-value-bp/bp.conf.js | 11 +++ benchmarks/select-ng-value-bp/main.html | 15 ++++ 3 files changed, 130 insertions(+) create mode 100755 benchmarks/select-ng-value-bp/app.js create mode 100755 benchmarks/select-ng-value-bp/bp.conf.js create mode 100755 benchmarks/select-ng-value-bp/main.html diff --git a/benchmarks/select-ng-value-bp/app.js b/benchmarks/select-ng-value-bp/app.js new file mode 100755 index 000000000000..033f55c23a27 --- /dev/null +++ b/benchmarks/select-ng-value-bp/app.js @@ -0,0 +1,104 @@ +"use strict"; + +/* globals angular, benchmarkSteps */ + +var app = angular.module('selectBenchmark', []); + +app.config(function($compileProvider) { + if ($compileProvider.debugInfoEnabled) { + $compileProvider.debugInfoEnabled(false); + } +}); + + + +app.controller('DataController', function($scope, $element) { + $scope.groups = []; + $scope.count = 10000; + + function changeOptions() { + $scope.groups = []; + var i = 0; + var group; + while(i < $scope.count) { + if (i % 100 === 0) { + group = { + name: 'group-' + $scope.groups.length, + items: [] + }; + $scope.groups.push(group); + } + group.items.push({ + id: i, + label: 'item-' + i + }); + i++; + } + } + + var selectElement = $element.find('select'); + console.log(selectElement); + + + benchmarkSteps.push({ + name: 'add-options', + fn: function() { + $scope.$apply(function() { + $scope.count = 10000; + changeOptions(); + }); + } + }); + + benchmarkSteps.push({ + name: 'set-model-1', + fn: function() { + $scope.$apply(function() { + $scope.x = $scope.groups[10].items[0]; + }); + } + }); + + benchmarkSteps.push({ + name: 'set-model-2', + fn: function() { + $scope.$apply(function() { + $scope.x = $scope.groups[0].items[10]; + }); + } + }); + + benchmarkSteps.push({ + name: 'remove-options', + fn: function() { + $scope.count = 100; + changeOptions(); + } + }); + + benchmarkSteps.push({ + name: 'add-options', + fn: function() { + $scope.$apply(function() { + $scope.count = 10000; + changeOptions(); + }); + } + }); + + benchmarkSteps.push({ + name: 'set-view-1', + fn: function() { + selectElement.val('2000'); + selectElement.triggerHandler('change'); + } + }); + + benchmarkSteps.push({ + name: 'set-view-2', + fn: function() { + selectElement.val('1000'); + selectElement.triggerHandler('change'); + } + }); +}); diff --git a/benchmarks/select-ng-value-bp/bp.conf.js b/benchmarks/select-ng-value-bp/bp.conf.js new file mode 100755 index 000000000000..bf543bb2cef7 --- /dev/null +++ b/benchmarks/select-ng-value-bp/bp.conf.js @@ -0,0 +1,11 @@ +module.exports = function(config) { + config.set({ + scripts: [ { + id: 'angular', + src: '/build/angular.js' + }, + { + src: 'app.js', + }] + }); +}; diff --git a/benchmarks/select-ng-value-bp/main.html b/benchmarks/select-ng-value-bp/main.html new file mode 100755 index 000000000000..273027615288 --- /dev/null +++ b/benchmarks/select-ng-value-bp/main.html @@ -0,0 +1,15 @@ +
+
+
+

+ Tests the execution of a select with ngRepeat'ed options with ngValue for rendering during model + and option updates. +

+ +
+
+