From 2420c0878a52645b8c1cdd37ecedc01e613b725c Mon Sep 17 00:00:00 2001 From: Martin Staffa Date: Mon, 11 Jul 2016 15:39:19 +0200 Subject: [PATCH] fix(ngOptions): remove selected attribute from unselected options When the select model changes, we add the "selected" attribute to the selected option, so that screen readers know which option is selected. Previously, we failed to remove the attribute from the selected / empty option when the model changed to match a different option, or the unknown / empty option. When using "track by", the behavior would also show when a user selected an option, and then the model was changed, because track by watches the tracked expression, and calls the $render function on change. This fix reads the current select value, finds the matching option and removes the "selected" attribute. IE9 had to be special cased, as it will report option.hasAttribute('selected') === true even if the option's property and attribute have been unset (even the dev tools show not selected attribute). I've added a custom matcher that accounts for this behavior. In all other browsers, property and attribute should always be in the same state. Since few people will use screen readers with IE9, I hope this is a satisfactory solution to the problem. Fixes #14892 Fixes #14419 Related #12731 --- src/ng/directive/ngOptions.js | 9 ++- test/ng/directive/ngOptionsSpec.js | 104 +++++++++++++++++++++++++++++ 2 files changed, 112 insertions(+), 1 deletion(-) diff --git a/src/ng/directive/ngOptions.js b/src/ng/directive/ngOptions.js index 10d970097f02..781d68b423d1 100644 --- a/src/ng/directive/ngOptions.js +++ b/src/ng/directive/ngOptions.js @@ -448,10 +448,11 @@ var ngOptionsDirective = ['$compile', '$document', '$parse', function($compile, var removeEmptyOption = function() { if (!providedEmptyOption) { emptyOption.remove(); + } else { + emptyOption.removeAttr('selected'); } }; - var renderUnknownOption = function() { selectElement.prepend(unknownOption); selectElement.val('?'); @@ -467,8 +468,13 @@ var ngOptionsDirective = ['$compile', '$document', '$parse', function($compile, if (!multiple) { selectCtrl.writeValue = function writeNgOptionsValue(value) { + var selectedOption = options.selectValueMap[selectElement.val()]; var option = options.getOptionFromViewValue(value); + // Make sure to remove the selected attribute from the previously selected option + // Otherwise, screen readers might get confused + if (selectedOption) selectedOption.element.removeAttribute('selected'); + if (option) { // Don't update the option when it is already selected. // For example, the browser will select the first option by default. In that case, @@ -509,6 +515,7 @@ var ngOptionsDirective = ['$compile', '$document', '$parse', function($compile, // If we are using `track by` then we must watch the tracked value on the model // since ngModel only watches for object identity change + // FIXME: When a user selects an option, this watch will fire needlessly if (ngOptions.trackBy) { scope.$watch( function() { return ngOptions.getTrackByValue(ngModelCtrl.$viewValue); }, diff --git a/test/ng/directive/ngOptionsSpec.js b/test/ng/directive/ngOptionsSpec.js index 8fb6a2a9fa3f..6eab57ff8fa3 100644 --- a/test/ng/directive/ngOptionsSpec.js +++ b/test/ng/directive/ngOptionsSpec.js @@ -120,6 +120,55 @@ describe('ngOptions', function() { return { pass: errors.length === 0, message: message }; } }; + }, + toBeMarkedAsSelected: function() { + // Selected is special because the element property and attribute reflect each other's state. + // IE9 will wrongly report hasAttribute('selected') === true when the property is + // undefined or null, and the dev tools show that no attribute is set + return { + compare: function(actual) { + var errors = []; + if (actual.selected === null || typeof actual.selected === 'undefined' || actual.selected === false) { + errors.push('Expected option property "selected" to be truthy'); + } + + if (msie !== 9 && actual.hasAttribute('selected') === false) { + errors.push('Expected option to have attribute "selected"'); + } + + var message = function() { + return errors.join('\n'); + }; + + var result = { + pass: errors.length === 0, + message: message() + }; + + return result; + }, + negativeCompare: function(actual) { + var errors = []; + if (actual.selected) { + errors.push('Expected option property "selected" to be falsy'); + } + + if (msie !== 9 && actual.hasAttribute('selected')) { + errors.push('Expected option not to have attribute "selected"'); + } + + var message = function() { + return errors.join('\n'); + }; + + var result = { + pass: errors.length === 0, + message: message() + }; + + return result; + } + }; } }); }); @@ -744,6 +793,41 @@ describe('ngOptions', function() { }); + it('should remove the "selected" attribute from the previous option when the model changes', function() { + scope.values = [{id: 10, label: 'ten'}, {id:20, label: 'twenty'}]; + + createSelect({ + 'ng-model': 'selected', + 'ng-options': 'item.label for item in values' + }, true); + + var options = element.find('option'); + expect(options[0]).toBeMarkedAsSelected(); + expect(options[1]).not.toBeMarkedAsSelected(); + expect(options[2]).not.toBeMarkedAsSelected(); + + scope.selected = scope.values[0]; + scope.$digest(); + + expect(options[0]).not.toBeMarkedAsSelected(); + expect(options[1]).toBeMarkedAsSelected(); + expect(options[2]).not.toBeMarkedAsSelected(); + + scope.selected = scope.values[1]; + scope.$digest(); + + expect(options[0]).not.toBeMarkedAsSelected(); + expect(options[1]).not.toBeMarkedAsSelected(); + expect(options[2]).toBeMarkedAsSelected(); + + scope.selected = 'no match'; + scope.$digest(); + + expect(options[0]).toBeMarkedAsSelected(); + expect(options[1]).not.toBeMarkedAsSelected(); + expect(options[2]).not.toBeMarkedAsSelected(); + }); + describe('disableWhen expression', function() { describe('on single select', function() { @@ -1395,6 +1479,26 @@ describe('ngOptions', function() { }); }).not.toThrow(); }); + + it('should remove the "selected" attribute when the model changes', function() { + createSelect({ + 'ng-model': 'selected', + 'ng-options': 'item.label for item in arr track by item.id' + }); + + var options = element.find('option'); + browserTrigger(options[2], 'click'); + + expect(scope.selected).toEqual(scope.arr[1]); + + scope.selected = {}; + scope.$digest(); + + expect(options[0]).toBeMarkedAsSelected(); + expect(options[2]).not.toBeMarkedAsSelected(); + expect(options[2]).not.toBeMarkedAsSelected(); + }); + });