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 angular#13583

Author: Narretz

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);
+      };
+    }
 };
 
 

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('<form name="form">' + html + '</form>');
@@ -10,10 +10,42 @@ describe('select', function() {
     scope.$apply();
   }
 
+  function compileRepeatedOptions() {
+    compile('<select ng-model="robot">' +
+              '<option value="{{item.value}}" ng-repeat="item in robots">{{item.label}}</option>' +
+            '</select>');
+  }
+
+  function compileGroupedOptions() {
+    compile(
+      '<select ng-model="mySelect">' +
+        '<option ng-repeat="item in values">{{item.name}}</option>' +
+        '<optgroup ng-repeat="group in groups" label="{{group.name}}">' +
+          '<option ng-repeat="item in group.values">{{item.name}}</option>' +
+        '</optgroup>' +
+      '</select>');
+  }
+
   function unknownValue(value) {
     return '? ' + hashKey(value) + ' ?';
   }
 
+  beforeEach(module(function($compileProvider) {
+    $compileProvider.directive('spyOnWriteValue', function() {
+      return {
+        require: 'select',
+        link: {
+          pre: function(scope, element, attrs, ctrl) {
+            selectCtrl = ctrl;
+            renderSpy = jasmine.createSpy('renderSpy');
+            selectCtrl.ngModelCtrl.$render = renderSpy.andCallFake(selectCtrl.ngModelCtrl.$render);
+            spyOn(selectCtrl, 'writeValue').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 +79,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 +232,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(
+        '<select ng-model="mySelect">' +
+          '<optgroup label="first">' +
+            '<option value="A">A</option>' +
+        '</optgroup>' +
+        '<optgroup label="second">' + '\n' +
+            '<option value="B">B</option>' +
+        '</optgroup>      ' +
+      '</select>');
+
+      $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(
+        '<select spy-on-write-value ng-model="mySelect">' +
+          '<optgroup label="first">' +
+            '<option value="A">A</option>' +
+        '</optgroup>' +
+        '<optgroup label="second">' + '\n' +
+            '<option value="B">B</option>' +
+        '</optgroup>      ' +
+      '</select>');
+
+      $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 +616,6 @@ describe('select', function() {
 
   describe('selectController.hasOption', function() {
 
-    function compileRepeatedOptions() {
-      compile('<select ng-model="robot">' +
-                '<option value="{{item.value}}" ng-repeat="item in robots">{{item.label}}</option>' +
-              '</select>');
-    }
-
-    function compileGroupedOptions() {
-      compile(
-        '<select ng-model="mySelect">' +
-          '<option ng-repeat="item in values">{{item.name}}</option>' +
-          '<optgroup ng-repeat="group in groups" label="{{group.name}}">' +
-            '<option ng-repeat="item in group.values">{{item.name}}</option>' +
-          '</optgroup>' +
-        '</select>');
-    }
-
     describe('flat options', function() {
       it('should return false for options shifted via ngRepeat', function() {
         scope.robots = [
@@ -624,7 +686,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 +727,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 +764,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 +803,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 +840,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 +875,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 +910,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']});
       });
     });
   });