-
Notifications
You must be signed in to change notification settings - Fork 160
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
igx-carousel accessibility improvements #8864
Conversation
…into ddincheva/carouselAria
…into ddincheva/carouselAria
…into ddincheva/carouselAria
<div *ngFor="let slide of slides" | ||
class="igx-carousel-indicators__indicator" | ||
(click)="select(slide)" | ||
[id]="setId(slide)" |
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.
Use a getter instead of a method for the attributes. Same is applicable to attr.aria-label
. You can host bind from within the component also with a getter.
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.
I agree with you that there is no need to use a method here, but I can not use a getter because the id and the labels that are constructed depends on the slide index and I can not pass a parameter to a getter, but maybe is better here to just construct the string literal directly in the template ... I don't see any benefit in using a function for that simple purpose. I can no use a host binding because tab elements are not in a separated component, they are part of the carousel component template.
|
||
/** @hidden */ | ||
@HostBinding('attr.aria-labelledby') | ||
public labelId = `igxCarouselLbl`; // tab-${this.current + 1}-3 |
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.
Should this be static?
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.
this is something that we discussed while she was addressing some in-progress stuff, this will become:
public labelId = `${this.id}-label`;
the template will be updated accordingly
public roleDescription = 'carousel'; | ||
|
||
/** @hidden */ | ||
@HostBinding('attr.aria-labelledby') |
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.
Let's set this aria attr only when the label is visible - which is happening when the total slides are more than the maximumIndicatorsCount
(5)
* ``` | ||
* | ||
*/ | ||
@HostBinding('attr.aria-selected') | ||
public get ariaLive() { | ||
return this.active ? 'polite' : null; | ||
public get ariaSelected(): boolean { |
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.
As for the aria-selected error of the slide element, it appears that when aria-selected is used, its controlled tabpanel
should have its aria-expanded
attribute set to true and its hidden attribute set to false, otherwise the reverse.
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.
aria-selected attribute should be added only to the element with role 'tab'. 'tabpanel' element should not have such an attribute /aria tabpanel spec/.. so I just removed it
Closes #8202
Additional information (check all that apply):
Checklist:
feature/README.MD
updates for the feature docsREADME.MD
CHANGELOG.MD
updates for newly added functionalityng update
migrations for the breaking changes (migrations guidelines)