feat(select): support values added by ngValue

Narretz · petebacondarwin · commit 71ff19485e66 · 2016-06-17T12:26:49.000+01:00
select elements with ngModel will now set ngModel to option values added by ngValue. This allows setting values of any type (not only strings) without the use of ngOptions. Interpolations inside attributes can only be strings, but the ngValue directive uses attrs.$set, which does not have any type restriction. Any $observe on the value attribute will therefore receive the original value (result of ngValue expression). However, when a user selects an option, the browser sets the select value to the actual option's value attribute, which is still always a string. For that reason, when options are added by ngValue, we set the hashed value of the original value in the value attribute and store the actual value in an extra map. When the select value changes, we read access the actual value via the hashed select value. Since we only use a hashed value for ngValue, we will have extra checks for the hashed values: - when options are read, for both single and multiple select - when options are written, for multiple select I don't expect this to have a performance impact, but it should be kept in mind. Closes angular#9842 Closes angular#6297 BREAKING CHANGE: `<option>` elements added to `<select ng-model>` via `ngValue` now add their values in hash form, i.e. `<option ng-value="myString">` becomes `<option ng-value="myString" value="string:myString">`. This is done to support binding options with values of any type to selects. This should rarely affect applications, as the values of options are usually not relevant to the application logic, but it's possible that option values are checked in tests.
diff --git a/src/ng/directive/select.js b/src/ng/directive/select.js
@@ -16,6 +16,8 @@ var SelectController =
   var self = this,
       optionsMap = new HashMap();
 
+  self.selectValueMap = {}; // Keys are the hashed values, values the original values
+
   // If the ngModel doesn't get provided then provide a dummy noop version to prevent errors
   self.ngModelCtrl = noopNgModelController;
 
@@ -47,7 +49,9 @@ var SelectController =
   // upon whether the select can have multiple values and whether ngOptions is at work.
   self.readValue = function readSingleValue() {
     self.removeUnknownOption();
-    return $element.val();
+    var val = $element.val();
+    // ngValue added option values are stored in the selectValueMap, normal interpolations are not
+    return val in self.selectValueMap ? self.selectValueMap[val] : val;
   };
 
 
@@ -106,9 +110,26 @@ var SelectController =
 
   self.registerOption = function(optionScope, optionElement, optionAttrs, interpolateValueFn, interpolateTextFn) {
 
-    if (interpolateValueFn) {
+    if (interpolateValueFn === true) {
+      // The value attribute is set by ngValue
+      var oldVal, hashedVal = NaN;
+      optionAttrs.$observe('value', function valueAttributeObserveAction(newVal) {
+        if (isDefined(hashedVal)) {
+          self.removeOption(oldVal);
+          delete self.selectValueMap[hashedVal];
+        }
+
+        hashedVal = hashKey(newVal);
+        oldVal = newVal;
+        self.addOption(newVal, optionElement);
+        self.selectValueMap[hashedVal] = newVal;
+        // Set the attribute directly instead of using optionAttrs.$set - this stops the observer
+        // from firing a second time. Other $observers on value will also get the result of the
+        // ngValue expression, not the hashed value
+        optionElement.attr('value', hashedVal);
+      });
+    } else if (interpolateValueFn) {
       // The value attribute is interpolated
-      var oldVal;
       optionAttrs.$observe('value', function valueAttributeObserveAction(newVal) {
         if (isDefined(oldVal)) {
           self.removeOption(oldVal);
@@ -384,7 +405,8 @@ var selectDirective = function() {
           var array = [];
           forEach(element.find('option'), function(option) {
             if (option.selected) {
-              array.push(option.value);
+              var val = option.value;
+              array.push(val in selectCtrl.selectValueMap ? selectCtrl.selectValueMap[val] : val);
             }
           });
           return array;
@@ -394,7 +416,7 @@ var selectDirective = function() {
         selectCtrl.writeValue = function writeMultipleValue(value) {
           var items = new HashMap(value);
           forEach(element.find('option'), function(option) {
-            option.selected = isDefined(items.get(option.value));
+            option.selected = isDefined(items.get(option.value)) || isDefined(items.get(selectCtrl.selectValueMap[option.value]));
           });
         };
 
@@ -445,9 +467,13 @@ var optionDirective = ['$interpolate', function($interpolate) {
     restrict: 'E',
     priority: 100,
     compile: function(element, attr) {
-      if (isDefined(attr.value)) {
+      var interpolateValueFn;
+
+      if (isDefined(attr.ngValue)) {
+        interpolateValueFn = true;
+      } else if (isDefined(attr.value)) {
         // If the value attribute is defined, check if it contains an interpolation
-        var interpolateValueFn = $interpolate(attr.value, true);
+        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
diff --git a/test/ng/directive/selectSpec.js b/test/ng/directive/selectSpec.js
@@ -1,7 +1,7 @@
 'use strict';
 
 describe('select', function() {
-  var scope, formElement, element, $compile, ngModelCtrl, selectCtrl, renderSpy;
+  var scope, formElement, element, $compile, ngModelCtrl, selectCtrl, renderSpy, optionAttributesList = [];
 
   function compile(html) {
     formElement = jqLite('<form name="form">' + html + '</form>');
@@ -55,6 +55,18 @@ describe('select', function() {
             '</option>'
       };
     });
+
+    $compileProvider.directive('exposeAttributes', function() {
+      return {
+        require: '^^select',
+        link: {
+          pre: function(scope, element, attrs, ctrl) {
+            optionAttributesList.push(attrs);
+          }
+        }
+      };
+    });
+
   }));
 
   beforeEach(inject(function($rootScope, _$compile_) {
@@ -1224,5 +1236,202 @@ describe('select', function() {
       }).toThrowMinErr('ng','badname', 'hasOwnProperty is not a valid "option value" name');
     });
 
+    describe('with ngValue (and non-primitive values)', function() {
+
+      they('should set the option attribute and select it for value $prop', [
+          'string',
+          undefined,
+          1,
+          true,
+          null,
+          {prop: 'value'},
+          ['a'],
+          NaN
+        ], function(prop) {
+          scope.option1 = prop;
+          scope.selected = 'NOMATCH';
+
+          compile('<select ng-model="selected">' +
+            '<option ng-value="option1">{{option1}}</option>' +
+          '</select>');
+
+          scope.$digest();
+          expect(element.find('option').eq(0).val()).toBe('? string:NOMATCH ?');
+
+          scope.selected = prop;
+          scope.$digest();
+
+          expect(element.find('option').eq(0).val()).toBe(hashKey(prop));
+
+          // Reset
+          scope.selected = false;
+          scope.$digest();
+
+          expect(element.find('option').eq(0).val()).toBe('? boolean:false ?');
+
+          browserTrigger(element.find('option').eq(0));
+          if (typeof prop === 'number' && isNaN(prop)) {
+            expect(scope.selected).toBeNaN();
+          } else {
+            expect(scope.selected).toBe(prop);
+          }
+      });
+
+
+      they('should update the option attribute and select it for value $prop', [
+          'string',
+          undefined,
+          1,
+          true,
+          null,
+          {prop: 'value'},
+          ['a'],
+          NaN
+        ], function(prop) {
+          scope.option = prop;
+          scope.selected = 'NOMATCH';
+
+          compile('<select ng-model="selected">' +
+            '<option ng-value="option">{{option}}</option>' +
+          '</select>');
+
+          var selectController = element.controller('select');
+          spyOn(selectController, 'removeOption').and.callThrough();
+
+          scope.$digest();
+          expect(selectController.removeOption).not.toHaveBeenCalled();
+          expect(element.find('option').eq(0).val()).toBe('? string:NOMATCH ?');
+
+          scope.selected = prop;
+          scope.$digest();
+
+          expect(element.find('option').eq(0).val()).toBe(hashKey(prop));
+
+          scope.option = 'UPDATEDVALUE';
+          scope.$digest();
+
+          expect(selectController.removeOption.calls.count()).toBe(1);
+
+          // Updating the option value currently does not update the select model
+          if (typeof prop === 'number' && isNaN(prop)) {
+            expect(selectController.removeOption.calls.argsFor(0)[0]).toBeNaN();
+            expect(scope.selected).toBeNaN();
+          } else {
+            expect(selectController.removeOption.calls.argsFor(0)[0]).toBe(prop);
+            expect(scope.selected).toBe(prop);
+          }
+
+          expect(element.find('option').eq(0).prop('selected')).toBe(true);
+          expect(element.find('option').eq(1).val()).toBe('string:UPDATEDVALUE');
+
+          scope.selected = 'UPDATEDVALUE';
+          scope.$digest();
+
+          // expect(element.find('option').eq(0).prop('selected')).toBe(true); not selected in Chrome?
+          expect(element.find('option').eq(0).val()).toBe('string:UPDATEDVALUE');
+      });
+
+      it('should interact with custom attribute $observe and $set calls', function() {
+        var log = [], optionAttr;
+
+        compile('<select ng-model="selected">' +
+          '<option expose-attributes ng-value="option">{{option}}</option>' +
+        '</select>');
+
+        optionAttr = optionAttributesList[0];
+        optionAttr.$observe('value', function(newVal) {
+          log.push(newVal);
+        });
+
+        scope.option = 'init';
+        scope.$digest();
+
+        expect(log[0]).toBe('init');
+        expect(element.find('option').eq(1).val()).toBe('string:init');
+
+        optionAttr.$set('value', 'update');
+        expect(log[1]).toBe('update');
+        expect(element.find('option').eq(1).val()).toBe('string:update');
+
+      });
+
+      describe('and select[multiple]', function() {
+
+        it('should allow multiple selection', function() {
+            scope.options = {
+              a: 'string',
+              b: undefined,
+              c: 1,
+              d: true,
+              e: null,
+              f: {prop: 'value'},
+              g: ['a'],
+              h: NaN
+            };
+            scope.selected = [];
+
+            compile('<select multiple ng-model="selected">' +
+              '<option ng-value="options.a">{{options.a}}</option>' +
+              '<option ng-value="options.b">{{options.b}}</option>' +
+              '<option ng-value="options.c">{{options.c}}</option>' +
+              '<option ng-value="options.d">{{options.d}}</option>' +
+              '<option ng-value="options.e">{{options.e}}</option>' +
+              '<option ng-value="options.f">{{options.f}}</option>' +
+              '<option ng-value="options.g">{{options.g}}</option>' +
+              '<option ng-value="options.h">{{options.h}}</option>' +
+            '</select>');
+
+            scope.$digest();
+            expect(element).toEqualSelect(
+              'string:string',
+              'undefined:undefined',
+              'number:1',
+              'boolean:true',
+              'object:null',
+              'object:4',
+              'object:5',
+              'number:NaN'
+            );
+
+            scope.selected = ['string', 1];
+            scope.$digest();
+
+            expect(element.find('option').eq(0).prop('selected')).toBe(true);
+            expect(element.find('option').eq(2).prop('selected')).toBe(true);
+
+            browserTrigger(element.find('option').eq(1));
+            expect(scope.selected).toEqual([undefined]);
+
+            //reset
+            scope.selected = [];
+            scope.$digest();
+
+            forEach(element.find('option'), function(option) {
+              // browserTrigger can't produce click + ctrl, so set selection manually
+              jqLite(option).prop('selected', true);
+            });
+
+            browserTrigger(element, 'change');
+
+            var arrayVal = ['a'];
+            arrayVal.$$hashKey = 'object:5';
+
+            expect(scope.selected).toEqual([
+              'string',
+              undefined,
+              1,
+              true,
+              null,
+              {prop: 'value', $$hashKey: 'object:4'},
+              arrayVal,
+              NaN
+            ]);
+        });
+
+      });
+
+
+    });
+
   });
 });
