From d173dbbc0e8d0b2889312cd4e2f843d5bab61682 Mon Sep 17 00:00:00 2001
From: Martin Staffa <mjstaffa@gmail.com>
Date: Thu, 1 Dec 2016 12:04:19 +0100
Subject: [PATCH] fix(ngOptions): don't add comment nodes as empty options

When the "empty option" element contains a transclusion directive, the result of the compilation always includes a comment node. Since we are adding / removing the "selected" attribute on the empty option, we need to make sure it's an actual element.

To solve this, we take advantage of the fact the each option element has an option directive that tries to register the option with the selectController. With ngOptions, this registerOption function is normally noop'd since it's not possible to add dynamic options. Now if the result of the empty option compilation is a comment, we re-define the function so that it catches empty options when they are actually linked / rendered.

Related #15454
Closes #15459
---
 src/ng/directive/ngOptions.js      | 33 ++++++++++-
 src/ng/directive/select.js         | 19 ++++---
 test/ng/directive/ngOptionsSpec.js | 90 ++++++++++++++++++++++++++++++
 3 files changed, 131 insertions(+), 11 deletions(-)

diff --git a/src/ng/directive/ngOptions.js b/src/ng/directive/ngOptions.js
index 8d562082f983..7265a1f6e892 100644
--- a/src/ng/directive/ngOptions.js
+++ b/src/ng/directive/ngOptions.js
@@ -420,6 +420,7 @@ var ngOptionsDirective = ['$compile', '$document', '$parse', function($compile,
       // option when the viewValue does not match any of the option values.
       for (var i = 0, children = selectElement.children(), ii = children.length; i < ii; i++) {
         if (children[i].value === '') {
+          selectCtrl.hasEmptyOption = true;
           selectCtrl.emptyOption = children.eq(i);
           break;
         }
@@ -556,9 +557,35 @@ var ngOptionsDirective = ['$compile', '$document', '$parse', function($compile,
         // compile the element since there might be bindings in it
         $compile(selectCtrl.emptyOption)(scope);
 
-        // remove the class, which is added automatically because we recompile the element and it
-        // becomes the compilation root
-        selectCtrl.emptyOption.removeClass('ng-scope');
+        if (selectCtrl.emptyOption[0].nodeType === NODE_TYPE_COMMENT) {
+          // This means the empty option has currently no actual DOM node, probably because
+          // it has been modified by a transclusion directive.
+          selectCtrl.hasEmptyOption = false;
+
+          // Redefine the registerOption function, which will catch
+          // options that are added by ngIf etc. (rendering of the node is async because of
+          // lazy transclusion)
+          selectCtrl.registerOption = function(optionScope, optionEl) {
+            if (optionEl.val() === '') {
+              selectCtrl.hasEmptyOption = true;
+              selectCtrl.emptyOption = optionEl;
+              selectCtrl.emptyOption.removeClass('ng-scope');
+              // This ensures the new empty option is selected if previously no option was selected
+              ngModelCtrl.$render();
+
+              optionEl.on('$destroy', function() {
+                selectCtrl.hasEmptyOption = false;
+                selectCtrl.emptyOption = undefined;
+              });
+            }
+          };
+
+        } else {
+          // remove the class, which is added automatically because we recompile the element and it
+          // becomes the compilation root
+          selectCtrl.emptyOption.removeClass('ng-scope');
+        }
+
       }
 
       selectElement.empty();
diff --git a/src/ng/directive/select.js b/src/ng/directive/select.js
index 34f7c8a2a853..20271e2077aa 100644
--- a/src/ng/directive/select.js
+++ b/src/ng/directive/select.js
@@ -32,6 +32,14 @@ var SelectController =
   // to create it in <select> and IE barfs otherwise.
   self.unknownOption = jqLite(window.document.createElement('option'));
 
+  // The empty option is an option with the value '' that te application developer can
+  // provide inside the select. When the model changes to a value that doesn't match an option,
+  // it is selected - so if an empty option is provided, no unknown option is generated.
+  // However, the empty option is not removed when the model matches an option. It is always selectable
+  // and indicates that a "null" selection has been made.
+  self.hasEmptyOption = false;
+  self.emptyOption = undefined;
+
   self.renderUnknownOption = function(val) {
     var unknownVal = self.generateUnknownOptionValue(val);
     self.unknownOption.val(unknownVal);
@@ -55,13 +63,6 @@ var SelectController =
     if (self.unknownOption.parent()) self.unknownOption.remove();
   };
 
-  // The empty option is an option with the value '' that te application developer can
-  // provide inside the select. When the model changes to a value that doesn't match an option,
-  // it is selected - so if an empty option is provided, no unknown option is generated.
-  // However, the empty option is not removed when the model matches an option. It is always selectable
-  // and indicates that a "null" selection has been made.
-  self.emptyOption = undefined;
-
   self.selectEmptyOption = function() {
     if (self.emptyOption) {
       $element.val('');
@@ -70,7 +71,7 @@ var SelectController =
   };
 
   self.unselectEmptyOption = function() {
-    if (self.emptyOption) {
+    if (self.hasEmptyOption) {
       self.emptyOption.removeAttr('selected');
     }
   };
@@ -132,6 +133,7 @@ var SelectController =
 
     assertNotHasOwnProperty(value, '"option value"');
     if (value === '') {
+      self.hasEmptyOption = true;
       self.emptyOption = element;
     }
     var count = optionsMap.get(value) || 0;
@@ -148,6 +150,7 @@ var SelectController =
       if (count === 1) {
         optionsMap.remove(value);
         if (value === '') {
+          self.hasEmptyOption = false;
           self.emptyOption = undefined;
         }
       } else {
diff --git a/test/ng/directive/ngOptionsSpec.js b/test/ng/directive/ngOptionsSpec.js
index c00cd507dbf7..9ea12735a835 100644
--- a/test/ng/directive/ngOptionsSpec.js
+++ b/test/ng/directive/ngOptionsSpec.js
@@ -2524,6 +2524,96 @@ describe('ngOptions', function() {
       }
     );
 
+
+    it('should select the correct option after linking when the ngIf expression is initially falsy', function() {
+      scope.values = [
+        {name:'black'},
+        {name:'white'},
+        {name:'red'}
+      ];
+      scope.selected = scope.values[2];
+
+      expect(function() {
+        createSingleSelect('<option ng-if="isBlank" value="">blank</option>');
+        scope.$apply();
+      }).not.toThrow();
+
+      expect(element.find('option')[2]).toBeMarkedAsSelected();
+      expect(linkLog).toEqual(['linkNgOptions']);
+    });
+
+
+    it('should add / remove the "selected" attribute on empty option which has an initially falsy ngIf expression', function() {
+      scope.values = [
+        {name:'black'},
+        {name:'white'},
+        {name:'red'}
+      ];
+      scope.selected = scope.values[2];
+
+      createSingleSelect('<option ng-if="isBlank" value="">blank</option>');
+      scope.$apply();
+
+      expect(element.find('option')[2]).toBeMarkedAsSelected();
+
+      scope.$apply('isBlank = true');
+      expect(element.find('option')[0].value).toBe('');
+      expect(element.find('option')[0]).not.toBeMarkedAsSelected();
+
+      scope.$apply('selected = null');
+      expect(element.find('option')[0].value).toBe('');
+      expect(element.find('option')[0]).toBeMarkedAsSelected();
+
+      scope.selected = scope.values[1];
+      scope.$apply();
+      expect(element.find('option')[0].value).toBe('');
+      expect(element.find('option')[0]).not.toBeMarkedAsSelected();
+      expect(element.find('option')[2]).toBeMarkedAsSelected();
+    });
+
+
+    it('should add / remove the "selected" attribute on empty option which has an initially truthy ngIf expression when no option is selected', function() {
+      scope.values = [
+        {name:'black'},
+        {name:'white'},
+        {name:'red'}
+      ];
+      scope.isBlank = true;
+
+      createSingleSelect('<option ng-if="isBlank" value="">blank</option>');
+      scope.$apply();
+
+      expect(element.find('option')[0].value).toBe('');
+      expect(element.find('option')[0]).toBeMarkedAsSelected();
+
+      scope.selected = scope.values[2];
+      scope.$apply();
+      expect(element.find('option')[0]).not.toBeMarkedAsSelected();
+      expect(element.find('option')[3]).toBeMarkedAsSelected();
+    });
+
+
+    it('should add the "selected" attribute on empty option which has an initially falsy ngIf expression when no option is selected', function() {
+      scope.values = [
+        {name:'black'},
+        {name:'white'},
+        {name:'red'}
+      ];
+
+      createSingleSelect('<option ng-if="isBlank" value="">blank</option>');
+      scope.$apply();
+
+      expect(element.find('option')[0]).not.toBeMarkedAsSelected();
+
+      scope.isBlank = true;
+      scope.$apply();
+
+      expect(element.find('option')[0].value).toBe('');
+      expect(element.find('option')[0]).toBeMarkedAsSelected();
+      expect(element.find('option')[1]).not.toBeMarkedAsSelected();
+    });
+
+
     it('should not throw when a directive compiles the blank option before ngOptions is linked', function() {
       expect(function() {
         createSelect({