Skip to content
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.

Sign up for GitHub

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

Xander/carousel accessibility compliance #1416

Closed
wants to merge 13 commits into from
+10 −10
Closed

Xander/carousel accessibility compliance #1416

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
2e362ee
tabpanel role removed
xman343 Jul 18, 2023
d8ca41f
button labels
xman343 Jul 18, 2023
426a98f
OnlyDotsEx tabpanel role removed
xman343 Jul 18, 2023
b470b59
OnlyDots id typo fixed
xman343 Jul 18, 2023
4a5875c
Merge branch 'dev' into xander/carousel-accessibility-compliance
xman343 Jul 19, 2023
0f3b67e
Merge branch 'dev' into xander/carousel-accessibility-compliance
xman343 Jul 19, 2023
6805c2f
nav button labels -> aria-labels
xman343 Jul 20, 2023
faf0cd5
<ul> and <li> -> <div>
xman343 Jul 20, 2023
eb22965
unit test updated
xman343 Jul 20, 2023
2f5127d
Merge branch 'dev' into xander/carousel-accessibility-compliance
xman343 Jul 20, 2023
2c0c7e1
Merge branch 'dev' into xander/carousel-accessibility-compliance
xman343 Jul 20, 2023
89cafd6
redundant <div> removed
xman343 Jul 21, 2023
cb583f9
Merge branch 'dev' into xander/carousel-accessibility-compliance
xman343 Jul 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions examples/Carousel.onlyDots.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,8 +46,7 @@ export default () => {
{gradients.map(({ from, to }, index) => (
<li
key={index}
role='tabpanel'
id={`${id}-slide-${index}`}
id={`${id}--slide-${index}`}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why was this id changed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
image

Copy link
Contributor

@mayank99 mayank99 Jul 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
image

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

onClick={({
currentTarget: { clientWidth },
nativeEvent: { offsetX },
Expand Down
4 changes: 2 additions & 2 deletions packages/itwinui-react/src/core/Carousel/Carousel.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,12 +32,12 @@ it('should render in its most basic state', () => {
expect(root).toHaveClass('iui-carousel');
expect(root).toHaveAttribute('aria-roledescription', 'carousel');

const slider = container.querySelector('ol') as HTMLElement;
const slider = container.querySelector('div') as HTMLElement;
expect(slider).toHaveClass('iui-carousel-slider');

slider.childNodes.forEach((slide, index) => {
expect(slide).toHaveClass('iui-carousel-slider-item');
expect(slide).toHaveAttribute('role', 'tabpanel');
expect(slide).toHaveAttribute('role', 'group');
expect(slide).toHaveAttribute('aria-roledescription', 'slide');
expect(slide).toHaveTextContent(`${index + 1}`);
expect(slide).toHaveAttribute('id', `testcarousel--slide-${index}`);
Expand Down
2 changes: 2 additions & 0 deletions packages/itwinui-react/src/core/Carousel/CarouselNavigation.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ const PreviousButton = React.forwardRef((props, ref) => {

return (
<IconButton
aria-label='Previous'
styleType='borderless'
size='small'
tabIndex={-1}
Expand Down Expand Up @@ -92,6 +93,7 @@ const NextButton = React.forwardRef((props, ref) => {

return (
<IconButton
aria-label='Next'
styleType='borderless'
size='small'
tabIndex={-1}
Expand Down
6 changes: 3 additions & 3 deletions packages/itwinui-react/src/core/Carousel/CarouselSlide.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,14 +49,14 @@ export const CarouselSlide = React.forwardRef((props, ref) => {

return (
<Box
as='li'
as='div'
className={cx('iui-carousel-slider-item', className)}
role='tabpanel'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i believe you need role='group' when using aria-roledescription

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
image

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

role='group'
aria-roledescription='slide'
ref={refs}
{...rest}
>
{children}
</Box>
);
}) as PolymorphicForwardRefComponent<'li', CarouselSlideProps>;
}) as PolymorphicForwardRefComponent<'div', CarouselSlideProps>;
5 changes: 2 additions & 3 deletions packages/itwinui-react/src/core/Carousel/CarouselSlider.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ export const CarouselSlider = React.forwardRef((props, ref) => {
setSlideCount(items.length);
}, [items.length, setSlideCount]);

const sliderRef = React.useRef<HTMLOListElement>(null);
const sliderRef = React.useRef<HTMLDivElement>(null);
const refs = useMergedRefs(sliderRef, ref);

scrollToSlide.current = (
Expand Down Expand Up @@ -86,7 +86,6 @@ export const CarouselSlider = React.forwardRef((props, ref) => {

return (
<Box
as='ol'
aria-live='polite'
className={cx('iui-carousel-slider', className)}
ref={refs}
Expand All @@ -96,4 +95,4 @@ export const CarouselSlider = React.forwardRef((props, ref) => {
{items}
</Box>
);
}) as PolymorphicForwardRefComponent<'ol'>;
}) as PolymorphicForwardRefComponent<'div'>;