From 107ac8ca3c9eff90d6c969d80e6faa02a4d21336 Mon Sep 17 00:00:00 2001
From: Georgios Kalpakas <kalpakas.g@gmail.com>
Date: Thu, 8 Dec 2016 19:05:11 +0200
Subject: [PATCH 1/2] 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 #15477
---
 src/ng/directive/ngOptions.js      | 24 ++++++---
 test/ng/directive/ngOptionsSpec.js | 81 ++++++++++++++++++++++++++++++
 2 files changed, 97 insertions(+), 8 deletions(-)

diff --git a/src/ng/directive/ngOptions.js b/src/ng/directive/ngOptions.js
index 7265a1f6e892..ce39d3ee4118 100644
--- a/src/ng/directive/ngOptions.js
+++ b/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;
diff --git a/test/ng/directive/ngOptionsSpec.js b/test/ng/directive/ngOptionsSpec.js
index 9ea12735a835..7a3c4ba492bf 100644
--- a/test/ng/directive/ngOptionsSpec.js
+++ b/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();
+    });
   });
 
 

From 60f98c80c0d32b672c0d00749ab0026baf1c9fe3 Mon Sep 17 00:00:00 2001
From: Georgios Kalpakas <kalpakas.g@gmail.com>
Date: Thu, 15 Dec 2016 23:47:44 +0200
Subject: [PATCH 2/2] fixup! fix(ngOptions): do not unset the `selected`
 property unless necessary

---
 src/ng/directive/ngOptions.js | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/ng/directive/ngOptions.js b/src/ng/directive/ngOptions.js
index ce39d3ee4118..b94f940a28e1 100644
--- a/src/ng/directive/ngOptions.js
+++ b/src/ng/directive/ngOptions.js
@@ -505,11 +505,11 @@ 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 = value && value.map(getAndUpdateSelectedOption) || [];
+          var selectedOptions = values && values.map(getAndUpdateSelectedOption) || [];
 
           options.items.forEach(function(option) {
             if (option.element.selected && !includes(selectedOptions, option)) {