Skip to content

Commit

Permalink
fix(ngOptions): remove selected attribute from unselected options
Browse files Browse the repository at this point in the history
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 angular#14892
Fixes angular#14419
Related angular#12731
  • Loading branch information
Narretz committed Jul 23, 2016
1 parent 5fd42b6 commit fbf1d1e
Show file tree
Hide file tree
Showing 2 changed files with 112 additions and 1 deletion.
9 changes: 8 additions & 1 deletion src/ng/directive/ngOptions.js
Original file line number Diff line number Diff line change
Expand Up @@ -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('?');
Expand All @@ -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,
Expand Down Expand Up @@ -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); },
Expand Down
104 changes: 104 additions & 0 deletions test/ng/directive/ngOptionsSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
};
}
});
});
Expand Down Expand Up @@ -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() {
Expand Down Expand Up @@ -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();
});

});


Expand Down

0 comments on commit fbf1d1e

Please sign in to comment.