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` directive's
`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

Closes #13583
Closes #13663

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']});
       });
     });
   });