-
Notifications
You must be signed in to change notification settings - Fork 6.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(select): add aria-owns property #1898
Conversation
* Option IDs need to be unique across components, so this counter exists outside of | ||
* the component definition. | ||
*/ | ||
var _selectOptionCounter = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about?
/** Unique ID counter for md-option. */
let _uniqueIdCounter = 0;
@@ -291,6 +300,11 @@ export class MdSelect implements AfterContentInit, ControlValueAccessor, OnDestr | |||
this._subscriptions = []; | |||
} | |||
|
|||
/** Records option IDs to pass to the aria-owns property. */ | |||
private _setOptionIds() { | |||
this._optionIds = this.options.map((option) => option.id).join(' '); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: don't need parens around option
expect(selects[0].nativeElement.getAttribute('aria-owns')) | ||
.not.toEqual(selects[1].nativeElement.getAttribute('aria-owns'), | ||
`Expected select components to have unique option IDs in aria-owns attr.`); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need a test where you assert that the aria-owns
contains each of the options's IDs. Something like
expect(ariaOwns).toContain(options[0].id);
expect(ariaOwns).toContain(options[1].id);
...
@jelbourn Comments addressed! |
LGTM |
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
r: @jelbourn