8000 fix(ngOptions): do not unset the `selected` property unless necessary · Nikhil22/angular.js@4e143fc · GitHub
[go: up one dir, main page]
Skip to content

Commit 4e143fc

Browse files
gkalpakgkalpak
committed
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
1 parent 15d07c0 commit 4e143fc

File tree

2 files changed

+98
-9
lines changed

2 files changed

+98
-9
lines changed

‎src/ng/directive/ngOptions.js

Lines changed: 17 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -505,17 +505,17 @@ var ngOptionsDirective = ['$compile', '$document', '$parse', function($compile,
505505

506506
} else {
507507

508-
selectCtrl.writeValue = function writeNgOptionsMultiple(value) {
508+
selectCtrl.writeValue = function writeNgOptionsMultiple(values) {
509+
// Only set `<option>.selected` if necessary, in order to prevent some browsers from
510+
// scrolling to `<option>` elements that are outside the `<select>` element's viewport.
511+
512+
var selectedOptions = values && values.map(getAndUpdateSelectedOption) || [];
513+
509514
options.items.forEach(function(option) {
510-
option.element.selected = false;
515+
if (option.element.selected && !includes(selectedOptions, option)) {
516+
option.element.selected = false;
517+
}
511518
});
512-
513-
if (value) {
514-
value.forEach(function(item) {
515-
var option = options.getOptionFromViewValue(item);
516-
if (option) option.element.selected = true;
517-
});
518-
}
519519
};
520520

521521

@@ -605,6 +605,14 @@ var ngOptionsDirective = ['$compile', '$document', '$parse', function($compile,
605605
updateOptionElement(option, optionElement);
606606
}
607607

608+
function getAndUpdateSelectedOption(viewValue) {
609+
var option = options.getOptionFromViewValue(viewValue);
610+
var element = option && option.element;
611+
612+
if (element && !element.selected) element.selected = true;
613+
614+
return option;
615+
}
608616

609617
function updateOptionElement(option, element) {
610618
option.element = element;

‎test/ng/directive/ngOptionsSpec.js

Lines changed: 81 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2827,6 +2827,7 @@ describe('ngOptions', function() {
28272827
expect(scope.selected).toEqual(['0']);
28282828
});
28292829

2830+
28302831
it('should deselect all options when model is emptied', function() {
28312832
createMultiSelect();
28322833
scope.$apply(function() {
@@ -2841,6 +2842,86 @@ describe('ngOptions', function() {
28412842

28422843
expect(element.find('option')[0].selected).toEqual(false);
28432844
});
2845+
2846+
2847+
it('should not re-set the `selected` property if it already has the correct value', function() {
2848+
scope.values = [{name: 'A'}, {name: 'B'}];
2849+
createMultiSelect();
2850+
2851+
var options = element.find('option');
2852+
var optionsSetSelected = [];
2853+
var _selected = [];
2854+
2855+
// Set up spies
2856+
forEach(options, function(option, i) {
2857+
optionsSetSelected[i] = jasmine.createSpy('optionSetSelected' + i);
2858+
_selected[i] = option.selected;
2859+
Object.defineProperty(option, 'selected', {
2860+
get: function() { return _selected[i]; },
2861+
set: optionsSetSelected[i].and.callFake(function(value) { _selected[i] = value; })
2862+
});
2863+
});
2864+
2865+
// Select `optionA`
2866+
scope.$apply('selected = [values[0]]');
2867+
2868+
expect(optionsSetSelected[0]).toHaveBeenCalledOnceWith(true);
2869+
expect(optionsSetSelected[1]).not.toHaveBeenCalled();
2870+
expect(options[0].selected).toBe(true);
2871+
expect(options[1].selected).toBe(false);
2872+
optionsSetSelected[0].calls.reset();
2873+
optionsSetSelected[1].calls.reset();
2874+
2875+
// Select `optionB` (`optionA` remains selected)
2876+
scope.$apply('selected.push(values[1])');
2877+
2878+
expect(optionsSetSelected[0]).not.toHaveBeenCalled();
2879+
expect(optionsSetSelected[1]).toHaveBeenCalledOnceWith(true);
2880+
expect(options[0].selected).toBe(true);
2881+
expect(options[1].selected).toBe(true);
2882+
optionsSetSelected[0].calls.reset();
2883+
optionsSetSelected[1].calls.reset();
2884+
2885+
// Unselect `optionA` (`optionB` remains selected)
2886+
scope.$apply('selected.shift()');
2887+
2888+
expect(optionsSetSelected[0]).toHaveBeenCalledOnceWith(false);
2889+
expect(optionsSetSelected[1]).not.toHaveBeenCalled();
2890+
expect(options[0].selected).toBe(false);
2891+
expect(options[1].selected).toBe(true);
2892+
optionsSetSelected[0].calls.reset();
2893+
optionsSetSelected[1].calls.reset();
2894+
2895+
// Reselect `optionA` (`optionB` remains selected)
2896+
scope.$apply('selected.push(values[0])');
2897+
2898+
expect(optionsSetSelected[0]).toHaveBeenCalledOnceWith(true);
2899+
expect(optionsSetSelected[1]).not.toHaveBeenCalled();
2900+
expect(options[0].selected).toBe(true);
2901+
expect(options[1].selected).toBe(true);
2902+
optionsSetSelected[0].calls.reset();
2903+
optionsSetSelected[1].calls.reset();
2904+
2905+
// Unselect `optionB` (`optionA` remains selected)
2906+
scope.$apply('selected.shift()');
2907+
2908+
expect(optionsSetSelected[0]).not.toHaveBeenCalled();
2909+
expect(optionsSetSelected[1]).toHaveBeenCalledOnceWith(false);
2910+
expect(options[0].selected).toBe(true);
2911+
expect(options[1].selected).toBe(false);
2912+
optionsSetSelected[0].calls.reset();
2913+
optionsSetSelected[1].calls.reset();
2914+
2915+
// Unselect `optionA`
2916+
scope.$apply('selected.length = 0');
2917+
2918+
expect(optionsSetSelected[0]).toHaveBeenCalledOnceWith(false);
2919+
expect(optionsSetSelected[1]).not.toHaveBeenCalled();
2920+
expect(options[0].selected).toBe(false);
2921+
expect(options[1].selected).toBe(false);
2922+
optionsSetSelected[0].calls.reset();
2923+
optionsSetSelected[1].calls.reset();
2924+
});
28442925
});
28452926

28462927

0 commit comments

Comments
 (0)
0