fix(select): handle corner case of adding options via a custom directive

Under specific circumstances (e.g. adding options via a directive with `replace: true` and a structural directive in its template), an error occurred when trying to call `hasAttribute()` on a comment node (which doesn't support that method). This commit fixes it by filtering out comment nodes in the `addOption()` method. Fixes #13874 Closes #13878
angular · Jan 29, 2016 · ca5b27b · ca5b27b
1 parent 3d158f6
commit ca5b27b
Show file tree

Hide file tree

Showing 2 changed files with 30 additions and 4 deletions.
diff --git a/src/ng/directive/select.js b/src/ng/directive/select.js
@@ -80,6 +80,9 @@ var SelectController =
 
   // Tell the select control that an option, with the given value, has been added
   self.addOption = function(value, element) {
+    // Skip comment nodes, as they only pollute the `optionsMap`
+    if (element[0].nodeType === NODE_TYPE_COMMENT) return;
+
     assertNotHasOwnProperty(value, '"option value"');
     if (value === '') {
       self.emptyOption = element;
@@ -452,7 +455,6 @@ var optionDirective = ['$interpolate', function($interpolate) {
     restrict: 'E',
     priority: 100,
     compile: function(element, attr) {
-
       if (isDefined(attr.value)) {
         // If the value attribute is defined, check if it contains an interpolation
         var interpolateValueFn = $interpolate(attr.value, true);
@@ -466,7 +468,6 @@ var optionDirective = ['$interpolate', function($interpolate) {
       }
 
       return function(scope, element, attr) {
-
         // This is an optimization over using ^^ since we don't want to have to search
         // all the way to the root of the DOM for every single option element
         var selectCtrlName = '$selectController',

diff --git a/test/ng/directive/selectSpec.js b/test/ng/directive/selectSpec.js
@@ -44,6 +44,17 @@ describe('select', function() {
         }
       };
     });
+
+    $compileProvider.directive('myOptions', function() {
+      return {
+        scope: {myOptions: '='},
+        replace: true,
+        template:
+            '<option value="{{ option.value }}" ng-repeat="option in myOptions">' +
+              '{{ options.label }}' +
+            '</option>'
+      };
+    });
   }));
 
   beforeEach(inject(function($rootScope, _$compile_) {
@@ -313,7 +324,7 @@ describe('select', function() {
       });
 
 
-    it('should cope with a dynamic empty option added to a static empty option', function() {
+      it('should cope with a dynamic empty option added to a static empty option', function() {
         scope.dynamicOptions = [];
         scope.robot = 'x';
         compile('<select ng-model="robot">' +
@@ -340,7 +351,7 @@ describe('select', function() {
         scope.dynamicOptions = [];
         scope.$digest();
         expect(element).toEqualSelect(['']);
-    });
+      });
 
       it('should select the empty option when model is undefined', function() {
         compile('<select ng-model="robot">' +
@@ -611,6 +622,20 @@ describe('select', function() {
 
     });
 
+
+    it('should not break when adding options via a directive with `replace: true` '
+        + 'and a structural directive in its template',
+      function() {
+        scope.options = [
+          {value: '1', label: 'Option 1'},
+          {value: '2', label: 'Option 2'},
+          {value: '3', label: 'Option 3'}
+        ];
+        compile('<select ng-model="mySelect"><option my-options="options"></option></select>');
+
+        expect(element).toEqualSelect([unknownValue()], '1', '2', '3');
+      }
+    );
   });