Xander/carousel accessibility compliance #1416
Conversation
xman343
added
the
a11y
xman343
requested review from
mayank99 and
LostABike
and removed request for
a team
packages/itwinui-react/src/core/Carousel/CarouselNavigation.tsx
Outdated
Show resolved
Hide resolved
| @@ -51,7 +51,6 @@ export const CarouselSlide = React.forwardRef((props, ref) => { | |||
| <Box | |||
| as='li' | |||
| className={cx('iui-carousel-slider-item', className)} | |||
| role='tabpanel' | |||
There was a problem hiding this comment.
i believe you need role='group' when using aria-roledescription
There was a problem hiding this comment.
Adding that role causes MainExample and ControlledExample to fail. These errors were present before I started this PR as well, and removing the original tabpanel role got rid of them. If you think it's necessary, I can try to look for another way to fix these issues while keeping the group (or tabpanel) role.
There was a problem hiding this comment.
that's interesting, because APG suggests using role=group (check the heading "Basic carousel elements")
i believe it might be because we are using li. we could make it a generic div. if we do that, the parent ol would also need to be a div.
| @@ -46,8 +46,7 @@ export default () => { | |||
| {gradients.map(({ from, to }, index) => ( | |||
| <li | |||
| key={index} | |||
| role='tabpanel' | |||
| id={`${id}-slide-${index}`} | |||
| id={`${id}--slide-${index}`} | |||
There was a problem hiding this comment.
There was a typo in the example that caused it to throw an error. It was supposed to be --slide-, which is the format used in the CarouselDotsList component.
There was a problem hiding this comment.
hmm i think this behavior is actually wrong. CarouselDotsList is the component, whereas Carousel.onlyDots is the example; the latter should not rely on implementation details of the former.
i wonder if we even need id and aria-controls. since we removed role=tabpanel in #1416 (comment), i think role=tab can also be removed from CarouselDot. i will double check tomorrow.
you can also try to read through APG on your own and compare our markup with theirs.
There was a problem hiding this comment.
I removed role=tab from CarouselDot myself, and it generated these violations. I think we have to keep this role, unless something else can be changed that would allow us to remove it.
There was a problem hiding this comment.
so i've been looking into this and need more time. I think fixing Carousel accessibility would require some bigger changes across the whole component, so you could move this PR to draft and work on a different PR in the meantime.
i will pick up Carousel later
|
There's a unit test that's failing, so I'm going to work on that too. |
|
Done, just had to update a querySelector (ul -> div) and a role (tabpanel -> group) |
Changes
Bringing
Carouselcomponent to accessibility compliance by distributing proper roles and labels within the component.Addresses #1148.
Testing
Evaluated using Cypress a11y tests. Unit & visual testing conducted as needed.
Docs
N/A