fix(ngOptions): remove selected attribute from unselected options #14894
Conversation
| 'ng-options': 'item.label for item in values' | ||
| }, true); | ||
|
|
||
| var resetButton = angular.element('<input type="reset" />'); |
There was a problem hiding this comment.
Isn't the reset button irrelevant. If so, I would avoid complicating the testcase.
Narretz
force-pushed
the
fix-select-selected
branch
5 times, most recently
from
d799025 to
a4f768f
Compare
|
Sorry for the potential ton of update emails you got, @gkalpak But now it's ready for another review |
| scope.selected = 'no match'; | ||
| scope.$digest(); | ||
|
|
||
| expect(options[0].selected).toBe(true); |
There was a problem hiding this comment.
Is this necessary? Isn't .toBeMarkedAsSelected() enough?
|
A couple of minor comments/questions. Otherwise LGTM 👍 |
| @@ -446,12 +446,12 @@ var ngOptionsDirective = ['$compile', '$document', '$parse', function($compile, | |||
| }; | |||
|
|
|||
| var removeEmptyOption = function() { | |||
| emptyOption.removeAttr('selected'); | |||
There was a problem hiding this comment.
Minor (as it isn't expected to happen often in an app's lifecycle), but this could be moved to an else branch (since we only care about the attribute if we aren't removing the element).
|
Do the tests fail now without the |
Narretz
force-pushed
the
fix-select-selected
branch
3 times, most recently
from
fbf1d1e to
2420c08
Compare
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
Narretz
force-pushed
the
fix-select-selected
branch
from
2420c08 to
04f711c
Compare
|
Yep, the tests fail now, when I remove the remoeAttr code. Thanks for the review! |
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
PR (#14894)
|
Thanks for working on this. However, I noticed it is still an issue in AngularJS 1.5.11. When I use ngOptions with track by the selected attributes for the unselected options are not removed.
Here is a link to Plnkr example: |
|
It happens on latest master too. @dustin-page can you please open a new issue. This has probably something to do with how you use |
|
using 1.6.8 and got same error when using track by: changing the selected element adds the selected attribute to the current item but doesn't remove it from the old one. |
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
bug fix
What is the current behavior? (You can also link to an open issue here)
See #14892 nad #14419
Does this PR introduce a breaking change?
No
Please check if the PR fulfills these requirements
[ ] Docs have been added / updated (for bug fixes / features)Other information:
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 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.
Fixes #14892
Fixes #14419
Related #12731