fix(select): add attribute "selected" for select[multiple]

This helps screen readers identify the selected options, see angular#14419
Narretz · Feb 8, 2017 · 0684b39 · 0684b39
1 parent 1078ec3
commit 0684b39
Show file tree

Hide file tree

Showing 2 changed files with 33 additions and 15 deletions.
diff --git 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
@@ -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() {