From be3a2b96be3bb81e8ea10e8d253b17bf498ced33 Mon Sep 17 00:00:00 2001 From: Martin Staffa Date: Fri, 18 Dec 2015 15:49:39 +0100 Subject: [PATCH] fix(select): re-define ngModelCtrl.$render in the select postLink fn Previously, the $render function was re-defined in the select directive's preLink function. When a select element is compiled, every option element inside it is linked and registered with the selectCtrl, which calls $render to update the selected option.$render calls selectCtrl.writeValue, which adds an unknown option in case no option is selected. In cases where optgroup elements are followed by a line-break, adding the unknown option confuses the html compiler and makes it call the link function of the following option with a wrong element, which means this option is not correctly registered. Since manipulation of the DOM in the preLink function is wrong API usage, the problem cannot be fixed in the compiler. With this commit, the $render function is not re-defined until the select postLink function, at which point all option elements have been linked already. The commit also changes the toEqualSelectWithOptions matcher to take selected options in groups into account. Closes #13583 --- src/ng/directive/select.js | 27 +++++--- test/ng/directive/selectSpec.js | 119 +++++++++++++++++++++++++------- 2 files changed, 113 insertions(+), 33 deletions(-) diff --git a/src/ng/directive/select.js b/src/ng/directive/select.js index 505fecfb410c..fdef4d779245 100644 --- a/src/ng/directive/select.js +++ b/src/ng/directive/select.js @@ -356,7 +356,8 @@ var selectDirective = function() { controller: SelectController, priority: 1, link: { - pre: selectPreLink + pre: selectPreLink, + post: selectPostLink } }; @@ -370,13 +371,6 @@ var selectDirective = function() { selectCtrl.ngModelCtrl = ngModelCtrl; - // We delegate rendering to the `writeValue` method, which can be changed - // if the select can have multiple selected values or if the options are being - // generated by `ngOptions` - ngModelCtrl.$render = function() { - selectCtrl.writeValue(ngModelCtrl.$viewValue); - }; - // When the selected item(s) changes we delegate getting the value of the select control // to the `readValue` method, which can be changed if the select can have multiple // selected values or if the options are being generated by `ngOptions` @@ -430,6 +424,23 @@ var selectDirective = function() { } } + + function selectPostLink(scope, element, attrs, ctrls) { + // if ngModel is not defined, we don't need to do anything + var ngModelCtrl = ctrls[1]; + if (!ngModelCtrl) return; + + var selectCtrl = ctrls[0]; + + // We delegate rendering to the `writeValue` method, which can be changed + // if the select can have multiple selected values or if the options are being + // generated by `ngOptions`. + // This must be done in the postLink fn to prevent $render to be called before + // all nodes have been linked correctly. + ngModelCtrl.$render = function() { + selectCtrl.writeValue(ngModelCtrl.$viewValue); + }; + } }; diff --git a/test/ng/directive/selectSpec.js b/test/ng/directive/selectSpec.js index 7be4cceb19f8..fdfae24d654e 100644 --- a/test/ng/directive/selectSpec.js +++ b/test/ng/directive/selectSpec.js @@ -1,7 +1,7 @@ 'use strict'; describe('select', function() { - var scope, formElement, element, $compile; + var scope, formElement, element, $compile, ngModelCtrl, selectCtrl, renderSpy; function compile(html) { formElement = jqLite('
' + html + '
'); @@ -10,10 +10,49 @@ describe('select', function() { scope.$apply(); } + function compileRepeatedOptions() { + compile(''); + } + + function compileGroupedOptions() { + compile( + ''); + } + function unknownValue(value) { return '? ' + hashKey(value) + ' ?'; } + beforeEach(module(function($compileProvider) { + $compileProvider.directive('captureSelectCtrl', function() { + return { + require: 'select', + link: { + pre: function(scope, element, attrs, ctrl) { + selectCtrl = ctrl; + renderSpy = jasmine.createSpy('renderSpy'); + dump('fakePre', selectCtrl.ngModelCtrl.$render); + selectCtrl.ngModelCtrl.$render = renderSpy.andCallFake(selectCtrl.ngModelCtrl.$render); + spyOn(selectCtrl, 'writeValue').andCallThrough(); + }, + post: function(scope, element, attrs, ctrl) { + dump('fakePost') + // expect(selectCtrl.ngModelCtrl.$render).not.toHaveBeenCalled(); + // dump('fakePost', selectCtrl.ngModelCtrl.$render) + // spyOn(selectCtrl.ngModelCtrl, '$render').andCallThrough(); + }, + } + }; + }); + })); + beforeEach(inject(function($rootScope, _$compile_) { scope = $rootScope.$new(); //create a child scope because the root scope can't be $destroy-ed $compile = _$compile_; @@ -47,12 +86,14 @@ describe('select', function() { toEqualSelectWithOptions: function(expected) { var actualValues = {}; var optionGroup; + var optionValue; forEach(this.actual.find('option'), function(option) { optionGroup = option.parentNode.label || ''; actualValues[optionGroup] = actualValues[optionGroup] || []; // IE9 doesn't populate the label property from the text property like other browsers - actualValues[optionGroup].push(option.label || option.text); + optionValue = option.label || option.text; + actualValues[optionGroup].push(option.selected ? [optionValue] : optionValue); }); this.message = function() { @@ -198,6 +239,50 @@ describe('select', function() { }); + it('should select options in a group when there is a linebreak before an option', function() { + scope.mySelect = 'B'; + scope.$apply(); + + var select = jqLite( + ''); + + $compile(select)(scope); + scope.$apply(); + + expect(select).toEqualSelectWithOptions({'first':['A'], 'second': [['B']]}); + dealoc(select); + }); + + + it('should only call selectCtrl.writeValue after a digest has occured', function() { + scope.mySelect = 'B'; + scope.$apply(); + + var select = jqLite( + ''); + + $compile(select)(scope); + expect(selectCtrl.writeValue).not.toHaveBeenCalled(); + + scope.$digest(); + expect(selectCtrl.writeValue).toHaveBeenCalledOnce(); + dealoc(select); + }); + describe('empty option', function() { it('should allow empty option to be added and removed dynamically', function() { @@ -538,22 +623,6 @@ describe('select', function() { describe('selectController.hasOption', function() { - function compileRepeatedOptions() { - compile(''); - } - - function compileGroupedOptions() { - compile( - ''); - } - describe('flat options', function() { it('should return false for options shifted via ngRepeat', function() { scope.robots = [ @@ -624,7 +693,7 @@ describe('select', function() { expect(selectCtrl.hasOption('A')).toBe(true); expect(selectCtrl.hasOption('B')).toBe(true); expect(selectCtrl.hasOption('C')).toBe(true); - expect(element).toEqualSelectWithOptions({'': ['A', 'B', 'C']}); + expect(element).toEqualSelectWithOptions({'': ['A', 'B', ['C']]}); }); }); @@ -665,7 +734,7 @@ describe('select', function() { expect(selectCtrl.hasOption('C')).toBe(true); expect(selectCtrl.hasOption('D')).toBe(true); expect(selectCtrl.hasOption('E')).toBe(true); - expect(element).toEqualSelectWithOptions({'': [''], 'first':['B', 'C'], 'second': ['D', 'E']}); + expect(element).toEqualSelectWithOptions({'': [['']], 'first':['B', 'C'], 'second': ['D', 'E']}); }); @@ -702,7 +771,7 @@ describe('select', function() { expect(selectCtrl.hasOption('C')).toBe(true); expect(selectCtrl.hasOption('D')).toBe(true); expect(selectCtrl.hasOption('E')).toBe(true); - expect(element).toEqualSelectWithOptions({'': [''], 'first':['B', 'C', 'D'], 'second': ['E']}); + expect(element).toEqualSelectWithOptions({'': [['']], 'first':['B', 'C', 'D'], 'second': ['E']}); }); @@ -741,7 +810,7 @@ describe('select', function() { expect(selectCtrl.hasOption('D')).toBe(true); expect(selectCtrl.hasOption('E')).toBe(true); expect(selectCtrl.hasOption('F')).toBe(false); - expect(element).toEqualSelectWithOptions({'': ['', 'A'], 'first':['B', 'C'], 'second': ['D', 'E']}); + expect(element).toEqualSelectWithOptions({'': [[''], 'A'], 'first':['B', 'C'], 'second': ['D', 'E']}); }); @@ -778,7 +847,7 @@ describe('select', function() { expect(selectCtrl.hasOption('C')).toBe(false); expect(selectCtrl.hasOption('D')).toBe(true); expect(selectCtrl.hasOption('E')).toBe(true); - expect(element).toEqualSelectWithOptions({'': ['', 'A'], 'first':['B', 'D'], 'second': ['E']}); + expect(element).toEqualSelectWithOptions({'': [[''], 'A'], 'first':['B', 'D'], 'second': ['E']}); }); @@ -813,7 +882,7 @@ describe('select', function() { expect(selectCtrl.hasOption('C')).toBe(true); expect(selectCtrl.hasOption('D')).toBe(false); expect(selectCtrl.hasOption('E')).toBe(true); - expect(element).toEqualSelectWithOptions({'': ['', 'A'], 'first':['B', 'C'], 'second': ['E']}); + expect(element).toEqualSelectWithOptions({'': [[''], 'A'], 'first':['B', 'C'], 'second': ['E']}); }); @@ -848,7 +917,7 @@ describe('select', function() { expect(selectCtrl.hasOption('C')).toBe(true); expect(selectCtrl.hasOption('D')).toBe(false); expect(selectCtrl.hasOption('E')).toBe(false); - expect(element).toEqualSelectWithOptions({'': ['', 'A'], 'first':['B', 'C']}); + expect(element).toEqualSelectWithOptions({'': [[''], 'A'], 'first':['B', 'C']}); }); }); });