fix(ngOptions): do not unset the selected property unless necessary

Previously, when updating the value of a `select[multiple]` element, all options
were first set to `selected = false` and then the selected ones where set to
`true`. By setting an already selected option to `selected = false` and then
`true` again - essentially unselecting and reselecting it - caused some browsers
(including Firefox, IE and (under some circumstances) Chrome) to scroll to
unexpectedly scroll to the last selected option.

This commit fixes it by ensuring that the `selected` property is set to its
final value and only if that value is different than the previous one, thus
avoiding the undesirable behavior.

Fixes angular#15477

Author: gkalpak

src/ng/directive/ngOptions.js

@@ -506,16 +506,16 @@ var ngOptionsDirective = ['$compile', '$document', '$parse', function($compile,
       } else {
 
         selectCtrl.writeValue = function writeNgOptionsMultiple(value) {
+          // Only set `<option>.selected` if necessary, in order to prevent some browsers from
+          // scrolling to `<option>` elements that are outside the `<select>` element's viewport.
+
+          var selectedOptions = value && value.map(getAndUpdateSelectedOption) || [];
+
           options.items.forEach(function(option) {
-            option.element.selected = false;
+            if (option.element.selected && !includes(selectedOptions, option)) {
+              option.element.selected = false;
+            }
           });
-
-          if (value) {
-            value.forEach(function(item) {
-              var option = options.getOptionFromViewValue(item);
-              if (option) option.element.selected = true;
-            });
-          }
         };
 
 
@@ -605,6 +605,14 @@ var ngOptionsDirective = ['$compile', '$document', '$parse', function($compile,
         updateOptionElement(option, optionElement);
       }
 
+      function getAndUpdateSelectedOption(viewValue) {
+        var option = options.getOptionFromViewValue(viewValue);
+        var element = option && option.element;
+
+        if (element && !element.selected) element.selected = true;
+
+        return option;
+      }
 
       function updateOptionElement(option, element) {
         option.element = element;

test/ng/directive/ngOptionsSpec.js

@@ -2827,6 +2827,7 @@ describe('ngOptions', function() {
       expect(scope.selected).toEqual(['0']);
     });
 
+
     it('should deselect all options when model is emptied', function() {
       createMultiSelect();
       scope.$apply(function() {
@@ -2841,6 +2842,86 @@ describe('ngOptions', function() {
 
       expect(element.find('option')[0].selected).toEqual(false);
     });
+
+
+    it('should not re-set the `selected` property if it already has the correct value', function() {
+      scope.values = [{name: 'A'}, {name: 'B'}];
+      createMultiSelect();
+
+      var options = element.find('option');
+      var optionsSetSelected = [];
+      var _selected = [];
+
+      // Set up spies
+      forEach(options, function(option, i) {
+        optionsSetSelected[i] = jasmine.createSpy('optionSetSelected' + i);
+        _selected[i] = option.selected;
+        Object.defineProperty(option, 'selected', {
+          get: function() { return _selected[i]; },
+          set: optionsSetSelected[i].and.callFake(function(value) { _selected[i] = value; })
+        });
+      });
+
+      // Select `optionA`
+      scope.$apply('selected = [values[0]]');
+
+      expect(optionsSetSelected[0]).toHaveBeenCalledOnceWith(true);
+      expect(optionsSetSelected[1]).not.toHaveBeenCalled();
+      expect(options[0].selected).toBe(true);
+      expect(options[1].selected).toBe(false);
+      optionsSetSelected[0].calls.reset();
+      optionsSetSelected[1].calls.reset();
+
+      // Select `optionB` (`optionA` remains selected)
+      scope.$apply('selected.push(values[1])');
+
+      expect(optionsSetSelected[0]).not.toHaveBeenCalled();
+      expect(optionsSetSelected[1]).toHaveBeenCalledOnceWith(true);
+      expect(options[0].selected).toBe(true);
+      expect(options[1].selected).toBe(true);
+      optionsSetSelected[0].calls.reset();
+      optionsSetSelected[1].calls.reset();
+
+      // Unselect `optionA` (`optionB` remains selected)
+      scope.$apply('selected.shift()');
+
+      expect(optionsSetSelected[0]).toHaveBeenCalledOnceWith(false);
+      expect(optionsSetSelected[1]).not.toHaveBeenCalled();
+      expect(options[0].selected).toBe(false);
+      expect(options[1].selected).toBe(true);
+      optionsSetSelected[0].calls.reset();
+      optionsSetSelected[1].calls.reset();
+
+      // Reselect `optionA` (`optionB` remains selected)
+      scope.$apply('selected.push(values[0])');
+
+      expect(optionsSetSelected[0]).toHaveBeenCalledOnceWith(true);
+      expect(optionsSetSelected[1]).not.toHaveBeenCalled();
+      expect(options[0].selected).toBe(true);
+      expect(options[1].selected).toBe(true);
+      optionsSetSelected[0].calls.reset();
+      optionsSetSelected[1].calls.reset();
+
+      // Unselect `optionB` (`optionA` remains selected)
+      scope.$apply('selected.shift()');
+
+      expect(optionsSetSelected[0]).not.toHaveBeenCalled();
+      expect(optionsSetSelected[1]).toHaveBeenCalledOnceWith(false);
+      expect(options[0].selected).toBe(true);
+      expect(options[1].selected).toBe(false);
+      optionsSetSelected[0].calls.reset();
+      optionsSetSelected[1].calls.reset();
+
+      // Unselect `optionA`
+      scope.$apply('selected.length = 0');
+
+      expect(optionsSetSelected[0]).toHaveBeenCalledOnceWith(false);
+      expect(optionsSetSelected[1]).not.toHaveBeenCalled();
+      expect(options[0].selected).toBe(false);
+      expect(options[1].selected).toBe(false);
+      optionsSetSelected[0].calls.reset();
+      optionsSetSelected[1].calls.reset();
+    });
   });