From c547ca45ec07f5b2f83e7c8507d79db051780ce5 Mon Sep 17 00:00:00 2001
From: Martin Staffa <mjstaffa@googlemail.com>
Date: Mon, 28 Sep 2015 23:39:21 +0200
Subject: [PATCH] fix(ngOptions): override select option registration

When ngOptions is present on a select, the option directive should not be able to
register options on the selectCtrl since this may cause errors during the
ngOptions lifecycle.

This can happen in the following cases:

- there is a blank option below the select element, an ngModel
directive, an ngOptions directive and some other directive on the select
element, which compiles the children of the select
(i.e. the option elements) before ngOptions is has finished linking.

- there is a blank option below the select element, an ngModel
directive, an ngOptions directive and another directive, which uses
templateUrl and replace:true.

What happens is:
- the option directive is compiled and adds an element `$destroy` listener
that will call `ngModel.$render` when the option element is removed.
- when `ngOptions` processes the option, it removes the element, and
triggers the `$destroy` listener on the option.
- the registered `$destroy` listener calls `$render` on `ngModel`.
- $render calls `selectCtrl.writeValue()`, which accesses the `options`
object in the `ngOptions` directive.
- Since `ngOptions` has not yet completed linking the `options` has not
yet been defined and we get an error.

This fix moves the registration code for the `option` directive into the
`SelectController`, which can then be easily overridden by the `ngOptions`
directive as a `noop`.

Fixes #11685
Closes #12972
Closes #12968
Closes #13012
---
 src/ng/directive/ngOptions.js      |  20 ++++--
 src/ng/directive/select.js         | 109 +++++++++++++++--------------
 test/ng/directive/ngOptionsSpec.js |  78 ++++++++++++++++++++-
 3 files changed, 147 insertions(+), 60 deletions(-)

diff --git a/src/ng/directive/ngOptions.js b/src/ng/directive/ngOptions.js
index 680472916359..f341d08a6267 100644
--- a/src/ng/directive/ngOptions.js
+++ b/src/ng/directive/ngOptions.js
@@ -392,11 +392,7 @@ var ngOptionsDirective = ['$compile', '$parse', function($compile, $parse) {
   var optionTemplate = document.createElement('option'),
       optGroupTemplate = document.createElement('optgroup');
 
-  return {
-    restrict: 'A',
-    terminal: true,
-    require: ['select', 'ngModel'],
-    link: function(scope, selectElement, attr, ctrls) {
+    function ngOptionsPostLink(scope, selectElement, attr, ctrls) {
 
       var selectCtrl = ctrls[0];
       var ngModelCtrl = ctrls[1];
@@ -448,7 +444,6 @@ var ngOptionsDirective = ['$compile', '$parse', function($compile, $parse) {
         unknownOption.remove();
       };
 
-
       // Update the controller methods for multiple selectable options
       if (!multiple) {
 
@@ -726,7 +721,20 @@ var ngOptionsDirective = ['$compile', '$parse', function($compile, $parse) {
         }
 
       }
+  }
 
+  return {
+    restrict: 'A',
+    terminal: true,
+    require: ['select', 'ngModel'],
+    link: {
+      pre: function ngOptionsPreLink(scope, selectElement, attr, ctrls) {
+        // Deactivate the SelectController.register method to prevent
+        // option directives from accidentally registering themselves
+        // (and unwanted $destroy handlers etc.)
+        ctrls[0].register = noop;
+      },
+      post: ngOptionsPostLink
     }
   };
 }];
diff --git a/src/ng/directive/select.js b/src/ng/directive/select.js
index df2bcee32437..111930dc23dd 100644
--- a/src/ng/directive/select.js
+++ b/src/ng/directive/select.js
@@ -2,6 +2,15 @@
 
 var noopNgModelController = { $setViewValue: noop, $render: noop };
 
+function chromeHack(optionElement) {
+  // Workaround for https://code.google.com/p/chromium/issues/detail?id=381459
+  // Adding an <option selected="selected"> element to a <select required="required"> should
+  // automatically select the new element
+  if (optionElement[0].hasAttribute('selected')) {
+    optionElement[0].selected = true;
+  }
+}
+
 /**
  * @ngdoc type
  * @name  select.SelectController
@@ -77,6 +86,8 @@ var SelectController =
     }
     var count = optionsMap.get(value) || 0;
     optionsMap.put(value, count + 1);
+    self.ngModelCtrl.$render();
+    chromeHack(element);
   };
 
   // Tell the select control that an option, with the given value, has been removed
@@ -98,6 +109,39 @@ var SelectController =
   self.hasOption = function(value) {
     return !!optionsMap.get(value);
   };
+
+
+  self.register = function($scope, $optionElement, $optionAttrs, interpolateValueFn, interpolateTextFn) {
+
+    if (interpolateValueFn) {
+      // The value attribute is interpolated
+      var oldVal;
+      $optionAttrs.$observe('value', function valueAttributeObserveAction(newVal) {
+        if (isDefined(oldVal)) {
+          self.removeOption(oldVal);
+        }
+        oldVal = newVal;
+        self.addOption(newVal, $optionElement);
+      });
+    } else if (interpolateTextFn) {
+      // The text content is interpolated
+      $scope.$watch(interpolateTextFn, function interpolateWatchAction(newVal, oldVal) {
+        $optionAttrs.$set('value', newVal);
+        if (oldVal !== newVal) {
+          self.removeOption(oldVal);
+        }
+        self.addOption(newVal, $optionElement);
+      });
+    } else {
+      // The value attribute is static
+      self.addOption($optionAttrs.value, $optionElement);
+    }
+
+    $optionElement.on('$destroy', function() {
+      self.removeOption($optionAttrs.value);
+      self.ngModelCtrl.$render();
+    });
+  };
 }];
 
 /**
@@ -308,7 +352,13 @@ var selectDirective = function() {
     restrict: 'E',
     require: ['select', '?ngModel'],
     controller: SelectController,
-    link: function(scope, element, attr, ctrls) {
+    priority: 1,
+    link: {
+      pre: selectPreLink
+    }
+  };
+
+  function selectPreLink(scope, element, attr, ctrls) {
 
       // if ngModel is not defined, we don't need to do anything
       var ngModelCtrl = ctrls[1];
@@ -378,7 +428,6 @@ var selectDirective = function() {
 
       }
     }
-  };
 };
 
 
@@ -386,16 +435,6 @@ var selectDirective = function() {
 // of dynamically created (and destroyed) option elements to their containing select
 // directive via its controller.
 var optionDirective = ['$interpolate', function($interpolate) {
-
-  function chromeHack(optionElement) {
-    // Workaround for https://code.google.com/p/chromium/issues/detail?id=381459
-    // Adding an <option selected="selected"> element to a <select required="required"> should
-    // automatically select the new element
-    if (optionElement[0].hasAttribute('selected')) {
-      optionElement[0].selected = true;
-    }
-  }
-
   return {
     restrict: 'E',
     priority: 100,
@@ -403,12 +442,12 @@ var optionDirective = ['$interpolate', function($interpolate) {
 
       if (isDefined(attr.value)) {
         // If the value attribute is defined, check if it contains an interpolation
-        var valueInterpolated = $interpolate(attr.value, true);
+        var interpolateValueFn = $interpolate(attr.value, true);
       } else {
         // If the value attribute is not defined then we fall back to the
         // text content of the option element, which may be interpolated
-        var interpolateFn = $interpolate(element.text(), true);
-        if (!interpolateFn) {
+        var interpolateTextFn = $interpolate(element.text(), true);
+        if (!interpolateTextFn) {
           attr.$set('value', element.text());
         }
       }
@@ -422,44 +461,8 @@ var optionDirective = ['$interpolate', function($interpolate) {
             selectCtrl = parent.data(selectCtrlName) ||
               parent.parent().data(selectCtrlName); // in case we are in optgroup
 
-        function addOption(optionValue) {
-          selectCtrl.addOption(optionValue, element);
-          selectCtrl.ngModelCtrl.$render();
-          chromeHack(element);
-        }
-
-        // Only update trigger option updates if this is an option within a `select`
-        // that also has `ngModel` attached
-        if (selectCtrl && selectCtrl.ngModelCtrl) {
-
-          if (valueInterpolated) {
-            // The value attribute is interpolated
-            var oldVal;
-            attr.$observe('value', function valueAttributeObserveAction(newVal) {
-              if (isDefined(oldVal)) {
-                selectCtrl.removeOption(oldVal);
-              }
-              oldVal = newVal;
-              addOption(newVal);
-            });
-          } else if (interpolateFn) {
-            // The text content is interpolated
-            scope.$watch(interpolateFn, function interpolateWatchAction(newVal, oldVal) {
-              attr.$set('value', newVal);
-              if (oldVal !== newVal) {
-                selectCtrl.removeOption(oldVal);
-              }
-              addOption(newVal);
-            });
-          } else {
-            // The value attribute is static
-            addOption(attr.value);
-          }
-
-          element.on('$destroy', function() {
-            selectCtrl.removeOption(attr.value);
-            selectCtrl.ngModelCtrl.$render();
-          });
+        if (selectCtrl) {
+          selectCtrl.register(scope, element, attr, interpolateValueFn, interpolateTextFn);
         }
       };
     }
diff --git a/test/ng/directive/ngOptionsSpec.js b/test/ng/directive/ngOptionsSpec.js
index bc50bcbef818..b96f8acc8026 100644
--- a/test/ng/directive/ngOptionsSpec.js
+++ b/test/ng/directive/ngOptionsSpec.js
@@ -2,7 +2,7 @@
 
 describe('ngOptions', function() {
 
-  var scope, formElement, element, $compile;
+  var scope, formElement, element, $compile, linkLog;
 
   function compile(html) {
     formElement = jqLite('<form name="form">' + html + '</form>');
@@ -104,6 +104,53 @@ describe('ngOptions', function() {
     });
   });
 
+  beforeEach(module(function($compileProvider, $provide) {
+    linkLog = [];
+
+    $compileProvider
+      .directive('customSelect', function() {
+        return {
+          restrict: "E",
+          replace: true,
+          scope: {
+            ngModel: '=',
+            options: '='
+          },
+          templateUrl: 'select_template.html',
+          link: function(scope, $element, attributes) {
+            scope.selectable_options = scope.options;
+          }
+        };
+      })
+
+      .directive('oCompileContents', function() {
+        return {
+          link: function(scope, element)  {
+            linkLog.push('linkCompileContents');
+            $compile(element.contents())(scope);
+          }
+        };
+    });
+
+    $provide.decorator('ngOptionsDirective', function($delegate) {
+
+      var origPreLink = $delegate[0].link.pre;
+      var origPostLink = $delegate[0].link.post;
+
+      $delegate[0].compile = function() {
+        return {
+          pre: origPreLink,
+          post: function() {
+            linkLog.push('linkNgOptions');
+            origPostLink.apply(this, arguments);
+          }
+        };
+      };
+
+      return $delegate;
+    });
+  }));
+
   beforeEach(inject(function($rootScope, _$compile_) {
     scope = $rootScope.$new(); //create a child scope because the root scope can't be $destroy-ed
     $compile = _$compile_;
@@ -2119,6 +2166,35 @@ describe('ngOptions', function() {
       option = element.find('option').eq(0);
       expect(option.text()).toBe('A');
     });
+
+
+    it('should not throw when a directive compiles the blank option before ngOptions is linked', function() {
+      expect(function() {
+        createSelect({
+          'o-compile-contents': '',
+          'name': 'select',
+          'ng-model': 'value',
+          'ng-options': 'item for item in items'
+        }, true);
+      }).not.toThrow();
+
+      expect(linkLog).toEqual(['linkCompileContents', 'linkNgOptions']);
+    });
+
+
+    it('should not throw with a directive that replaces', inject(function($templateCache, $httpBackend) {
+      $templateCache.put('select_template.html', '<select ng-options="option as option for option in selectable_options"> <option value="">This is a test</option> </select>');
+
+      scope.options = ['a', 'b', 'c', 'd'];
+
+      expect(function() {
+        element = $compile('<custom-select ng-model="value" options="options"></custom-select>')(scope);
+        scope.$digest();
+      }).not.toThrow();
+
+      dealoc(element);
+    }));
+
   });