From 1078ec3f3f279792760eb1bbfa9c574fe4da42bb Mon Sep 17 00:00:00 2001 From: Martin Staffa Date: Fri, 3 Feb 2017 17:24:57 +0100 Subject: [PATCH 1/2] fix(select): keep original selection when using shift to add options in IE/Edge In IE9-11 + Edge, the selected options were previously incorrect under the following circumstances: - at least two options are selected - shift+click or shift+down/up is used to add to the selection (any number of options) In these cases, only the last of the previously selected options and the newly selected options would be selected. The problems seems to be that the render engine gets confused when an option that already has selected = true gets selected = true set again. Note that this is not testable via unit test because it's not possible to simulate click / keyboard events on option elements (the events are delegated to the select element change event), and the problem also doesn't appear when modifying the option elements directly and then using the selectController API. It seems that this only happens when you manipulate the select directly in the user interface. Fixes #15675 Closes #15676 --- src/ng/directive/select.js | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/src/ng/directive/select.js b/src/ng/directive/select.js index 0c8201068c29..734fc49750e1 100644 --- a/src/ng/directive/select.js +++ b/src/ng/directive/select.js @@ -607,8 +607,17 @@ var selectDirective = function() { // Write value now needs to set the selected property of each matching option selectCtrl.writeValue = function writeMultipleValue(value) { forEach(element.find('option'), function(option) { - option.selected = !!value && (includes(value, option.value) || - includes(value, selectCtrl.selectValueMap[option.value])); + var shouldBeSelected = !!value && (includes(value, option.value) || + includes(value, selectCtrl.selectValueMap[option.value])); + var currentlySelected = option.selected; + + // IE and Edge will de-select selected options when you set the selected property again, e.g. + // when you add to the selection via shift+click/UP/DOWN + // Therefore, only set it if necessary + if ((shouldBeSelected && !currentlySelected) || (!shouldBeSelected && currentlySelected)) { + option.selected = shouldBeSelected; + } + }); }; From 0684b3997e5aed20db827b80b068dcaf862668c4 Mon Sep 17 00:00:00 2001 From: Martin Staffa Date: Sun, 5 Feb 2017 17:31:41 +0100 Subject: [PATCH 2/2] fix(select): add attribute "selected" for select[multiple] This helps screen readers identify the selected options, see #14419 --- src/ng/directive/select.js | 40 ++++++++++++++++++++------------- test/ng/directive/selectSpec.js | 8 +++++++ 2 files changed, 33 insertions(+), 15 deletions(-) diff --git a/src/ng/directive/select.js b/src/ng/directive/select.js index 734fc49750e1..4755a85932f5 100644 --- a/src/ng/directive/select.js +++ b/src/ng/directive/select.js @@ -4,6 +4,18 @@ var noopNgModelController = { $setViewValue: noop, $render: noop }; +function setOptionSelectedStatus(optionEl, value) { + optionEl.prop('selected', value); // needed for IE + /** + * When unselecting an option, setting the property to null / false should be enough + * However, screenreaders might react to the selected attribute instead, see + * https://github.com/angular/angular.js/issues/14419 + * Note: "selected" is a boolean attr and will be removed when the "value" arg in attr() is false + * or null + */ + optionEl.attr('selected', value); +} + /** * @ngdoc type * @name select.SelectController @@ -44,14 +56,14 @@ var SelectController = var unknownVal = self.generateUnknownOptionValue(val); self.unknownOption.val(unknownVal); $element.prepend(self.unknownOption); - setOptionAsSelected(self.unknownOption); + setOptionSelectedStatus(self.unknownOption, true); $element.val(unknownVal); }; self.updateUnknownOption = function(val) { var unknownVal = self.generateUnknownOptionValue(val); self.unknownOption.val(unknownVal); - setOptionAsSelected(self.unknownOption); + setOptionSelectedStatus(self.unknownOption, true); $element.val(unknownVal); }; @@ -66,7 +78,7 @@ var SelectController = self.selectEmptyOption = function() { if (self.emptyOption) { $element.val(''); - setOptionAsSelected(self.emptyOption); + setOptionSelectedStatus(self.emptyOption, true); } }; @@ -102,7 +114,7 @@ var SelectController = // Make sure to remove the selected attribute from the previously selected option // Otherwise, screen readers might get confused var currentlySelectedOption = $element[0].options[$element[0].selectedIndex]; - if (currentlySelectedOption) currentlySelectedOption.removeAttribute('selected'); + if (currentlySelectedOption) setOptionSelectedStatus(jqLite(currentlySelectedOption), false); if (self.hasOption(value)) { self.removeUnknownOption(); @@ -112,7 +124,7 @@ var SelectController = // Set selected attribute and property on selected option for screen readers var selectedOption = $element[0].options[$element[0].selectedIndex]; - setOptionAsSelected(jqLite(selectedOption)); + setOptionSelectedStatus(jqLite(selectedOption), true); } else { if (value == null && self.emptyOption) { self.removeUnknownOption(); @@ -292,11 +304,6 @@ var SelectController = } }); }; - - function setOptionAsSelected(optionEl) { - optionEl.prop('selected', true); // needed for IE - optionEl.attr('selected', true); - } }]; /** @@ -611,11 +618,14 @@ var selectDirective = function() { includes(value, selectCtrl.selectValueMap[option.value])); var currentlySelected = option.selected; - // IE and Edge will de-select selected options when you set the selected property again, e.g. - // when you add to the selection via shift+click/UP/DOWN - // Therefore, only set it if necessary - if ((shouldBeSelected && !currentlySelected) || (!shouldBeSelected && currentlySelected)) { - option.selected = shouldBeSelected; + // IE and Edge, adding options to the selection via shift+click/UP/DOWN, + // will de-select already selected options if "selected" on those options was set + // more than once (i.e. when the options were already selected) + // So we only modify the selected property if neccessary. + // Note: this behavior cannot be replicated via unit tests because it only shows in the + // actual user interface. + if (shouldBeSelected !== currentlySelected) { + setOptionSelectedStatus(jqLite(option), shouldBeSelected); } }); diff --git a/test/ng/directive/selectSpec.js b/test/ng/directive/selectSpec.js index e77d5bffcf3f..975e4b594d77 100644 --- a/test/ng/directive/selectSpec.js +++ b/test/ng/directive/selectSpec.js @@ -1098,13 +1098,21 @@ describe('select', function() { scope.selection = ['A']; }); + var optionElements = element.find('option'); + expect(element).toEqualSelect(['A'], 'B'); + expect(optionElements[0]).toBeMarkedAsSelected(); + expect(optionElements[1]).not.toBeMarkedAsSelected(); scope.$apply(function() { scope.selection.push('B'); }); + optionElements = element.find('option'); + expect(element).toEqualSelect(['A'], ['B']); + expect(optionElements[0]).toBeMarkedAsSelected(); + expect(optionElements[1]).toBeMarkedAsSelected(); }); it('should work with optgroups', function() {