feat(typeahead) : accessibility support #1321
Conversation
Fixes focus management and speech when using a screen reader
src/typeahead/typeahead-window.ts
Outdated
| * An optional id for the typeahead. The id should be unique. | ||
| * If not provided, it will be auto-generated. | ||
| */ | ||
| @HostBinding('attr.id') @Input() id: string = undefined; |
There was a problem hiding this comment.
Could you please use 'host' instead of @HostBinding so all the host bindings are in one place only?
There was a problem hiding this comment.
There is no need for = undefined - it is initialized like this by default.
There was a problem hiding this comment.
Also the comment seems to be invalid as we are not auto-generating id when one is not provided.
There was a problem hiding this comment.
The id is actually generated in the typeahead.ts file and set when the popup is opened (see _openPopup method).
It's actually kinda hackish but I don't know how else I can set the id of the popup. If I generate it in the typeahead-window.ts file, a new id will be generated each time the popup is opened (since a new instance is created each time)
I applied your other comments.
src/typeahead/typeahead-window.ts
Outdated
| <button type="button" class="dropdown-item" [class.active]="idx === activeIdx" | ||
| (mouseenter)="markActive(idx)" | ||
| <button type="button" class="dropdown-item" role="option" | ||
| [attr.id]="id + '-' + idx" |
There was a problem hiding this comment.
I think that you can use [id]="..." instead of [attr.id] as buttons should have the id property.
src/typeahead/typeahead-window.ts
Outdated
|
|
||
| next() { | ||
| if (this.activeIdx === this.results.length - 1) { | ||
| this.activeIdx = this.focusFirst ? (this.activeIdx + 1) % this.results.length : -1; | ||
| } else { | ||
| this.activeIdx++; | ||
| } | ||
| this.activeChangedEvent.emit(this.id + '-' + this.activeIdx); |
There was a problem hiding this comment.
You've got this this.activeChangedEvent.emit(this.id + '-' + this.activeIdx) logic repeated in several places, would be good to extract it. Also, shouldn't -1 by handled as a special case?
src/typeahead/typeahead.ts
Outdated
| @@ -124,6 +129,10 @@ export class NgbTypeahead implements ControlValueAccessor, | |||
| */ | |||
| @Output() selectItem = new EventEmitter<NgbTypeaheadSelectItemEvent>(); | |||
|
|
|||
| @HostBinding('attr.aria-activedescendant') activeDescendant: string = undefined; | |||
There was a problem hiding this comment.
activeDescendant: string is enough, no need for = undefined
src/typeahead/typeahead.ts
Outdated
| @@ -124,6 +129,10 @@ export class NgbTypeahead implements ControlValueAccessor, | |||
| */ | |||
| @Output() selectItem = new EventEmitter<NgbTypeaheadSelectItemEvent>(); | |||
|
|
|||
| @HostBinding('attr.aria-activedescendant') activeDescendant: string = undefined; | |||
| @HostBinding('attr.aria-owns') popupId: string = `ngb-typeahead-${nextId++}`; | |||
There was a problem hiding this comment.
Please use host instead of @HostBinding
There was a problem hiding this comment.
No need for type (: string) as TS will infer it
src/typeahead/typeahead.ts
Outdated
| @@ -124,6 +129,10 @@ export class NgbTypeahead implements ControlValueAccessor, | |||
| */ | |||
| @Output() selectItem = new EventEmitter<NgbTypeaheadSelectItemEvent>(); | |||
|
|
|||
| @HostBinding('attr.aria-activedescendant') activeDescendant: string = undefined; | |||
| @HostBinding('attr.aria-owns') popupId: string = `ngb-typeahead-${nextId++}`; | |||
| @HostBinding('attr.aria-expanded') isOpen: boolean = false; | |||
There was a problem hiding this comment.
No need for type (: boolean) as TS will infer it
|
@thbt thnx for the PR - I did the first pass on it and left some comments. Please address those and then I will have another look. Thnx! |
Fixes focus management and speech when using a screen reader