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 were 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 unexpectedly
scroll to the last selected option.

This commit fixes it by ensuring that the `selected` property is only set if its
current value is different than the new one and even then it is set to its final
value at once (i.e. without first setting it to `false`), thus avoiding the
undesirable behavior.

Fixes angular#15477

Closes angular#15478

Author: gkalpak

src/ng/directive/ngOptions.js

@@ -505,17 +505,17 @@ var ngOptionsDirective = ['$compile', '$document', '$parse', function($compile,
 
       } else {
 
-        selectCtrl.writeValue = function writeNgOptionsMultiple(value) {
+        selectCtrl.writeValue = function writeNgOptionsMultiple(values) {
+          // 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 = values && values.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();
+    });
   });