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

Issue #47: fix a11y eslint errors #53

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

Issue #47: fix a11y eslint errors #53

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
2286d6f
add roles
epramer-godaddy Oct 1, 2020
f2c7333
add onFocus
epramer-godaddy Oct 1, 2020
32986c7
ignore specific line for a11y
epramer-godaddy Oct 2, 2020
854f2ad
stub tests
epramer-godaddy Oct 2, 2020
ecb3c8a
flesh out tests
epramer-godaddy Oct 5, 2020
1cc74a2
finish tests
epramer-godaddy Oct 6, 2020
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
55 changes: 15 additions & 40 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

11 changes: 7 additions & 4 deletions src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,3 @@
/* eslint
jsx-a11y/mouse-events-have-key-events: 0,
jsx-a11y/no-noninteractive-element-interactions: 0,
jsx-a11y/click-events-have-key-events: 0 */
import React, { Component, Children, cloneElement } from 'react';
import PropTypes from 'prop-types';
import nth from 'lodash.nth';
Expand Down Expand Up @@ -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.

@msluther @thumbsupep this is probably a good example to base off of https://www.w3.org/TR/wai-aria-practices-1.1/examples/carousel/carousel-1.html

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

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?

onMouseEnter={ this.onMouseEnter }
onTouchStart={ this.onTouchStart }
>
Expand Down Expand Up @@ -565,7 +563,12 @@ export default class Carousel extends Component {
}

return (
// key handler is not needed due to the tab role - tab + enter
// performs the same function as onClick
// eslint-disable-next-line jsx-a11y/click-events-have-key-events
<li
role='tab'
aria-selected={ index === currentSlide }
key={ key }
style={ slideStyle }
data-index={ index }
Expand Down
40 changes: 40 additions & 0 deletions test/unit/carousel.tests.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -593,4 +593,44 @@ describe('Carousel', () => {
done();
}, 20);
});

describe('a11y', () => {
it('current slide has aria-selected', done => {
renderToJsdom(
<Carousel initialSlide={ 2 } slideWidth='300px' viewportWidth='300px' infinite={ false }>
<div id='slide1'/>
<div id='slide2'/>
<div id='slide3'/>
</Carousel>
);

setImmediate(() => {
const current = tree.find('.carousel-slide-selected');
const slides = tree.find('.carousel-slide');
slides.forEach((slide, i) => {
if (i !== 2) {
expect(slide.prop('aria-selected')).to.be.false;
}
});
expect(current.prop('aria-selected')).to.be.true;
done();
});
});

it('carousel-track has onFocus property', done => {
renderToJsdom(
<Carousel initialSlide={ 1 } slideWidth='300px' viewportWidth='300px' infinite={ false }>
<div id='slide1'/>
<div id='slide2'/>
<div id='slide3'/>
</Carousel>
);

setImmediate(() => {
const track = tree.find('.carousel-track');
expect(track.prop('onFocus')).to.be.a('function');
done();
});
});
});
});