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.

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

Issue #47: fix a11y eslint errors #53

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

thumbsupep
Copy link

addresses issue #47

Copy link
Contributor

@msluther msluther left a comment

Choose a reason for hiding this comment

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

Is the full carousel keyboard navigatable? Can you tab to the carousel and get through all the images in some reasonable way?

className='carousel-track'
style={ trackStyle }
ref={ t => { this._track = t; } }
onTransitionEnd={ this.slideTransitionEnd }
onMouseDown={ this.onMouseDown }
onMouseLeave={ this.onMouseLeave }
onMouseOver={ this.onMouseOver }
onFocus={ this.onMouseOver }
Copy link
Contributor

Choose a reason for hiding this comment

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

Would this also need an onBlur/onFocusOut handler as well?

@@ -471,13 +467,15 @@ export default class Carousel extends Component {
}
<div className='carousel-viewport' ref={ v => { this._viewport = v; } } style={ viewportStyle }>
<ul
role='tablist'
Copy link
Contributor

Choose a reason for hiding this comment

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

Is tablist/tab the right role here?

From https://www.w3.org/WAI/PF/HTML/wiki/RoleAttribute#ARIA_1.0_Pre-Defined_Roles

tab
A header for a tabpanel.

tablist
A list of tabs, which are references to tabpanels.

tabpanel
Tabpanel is a container for the resources associated with a tab.

Copy link
Author

Choose a reason for hiding this comment

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

it has to be an interactive role. I'm not sure where to find a comprehensive list of which ones are interactive - but I found this list https://github.com/jsx-eslint/eslint-plugin-jsx-a11y/blob/master/docs/rules/no-noninteractive-element-to-interactive-role.md#rule-details
and tab seems correct if you replace the word tab with slide in "selecting a different tab changes the content and makes the selected tab more prominent than the other tabs" https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Roles/Tab_Role

is there a better interactive role?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not opposed to this role, and it's certainly better than whats already there. My only concern is that there's no "content" being shown, just the "tab" itself. i.e. a tab is supposed to effectively swap out the primary tabpanel(or make it more prominent) and there's no tabpanel here.

Copy link
Contributor

Choose a reason for hiding this comment

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

If each image was a tabpanel and there were "dots" or some positional indicator that the user can click on/keyboard to activate a specific image, then those "dots" would be tabs. In the absence of dots/indicators, maybe tablist/tabpanel makes the most sense here?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is probably fine, but it might be a good idea to add aria-roledescription attributes of carousel and slide just to be a bit more descriptive for screen readers. I did see this page that recommended that: https://www.w3.org/TR/wai-aria-practices-1.1/examples/carousel/carousel-1.html

Base automatically changed from master to main February 3, 2021 15:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants