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

Jump to bottom

[Tabs] scrollButtons have an empty button error in compliance tools #15646

Merged
merged 4 commits into from May 12, 2019
Merged

[Tabs] scrollButtons have an empty button error in compliance tools #15646

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d5ac67c
TabScrollButton-Accessibility-Update
May 10, 2019
348af0a
Update TabScrollButton.js
May 10, 2019
719f867
make Wave and VoiceOver happy
oliviertassinari May 10, 2019
de2ce8d
💎👌
oliviertassinari May 10, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
11 changes: 9 additions & 2 deletions packages/material-ui/src/Tabs/TabScrollButton.js
View file
Open in desktop
Expand Up @@ -24,11 +24,18 @@ const TabScrollButton = React.forwardRef(function TabScrollButton(props, ref) {
const className = clsx(classes.root, classNameProp);

if (!visible) {
return <div className={className} />;
return <span className={className} />;
}

return (
<ButtonBase className={className} onClick={onClick} ref={ref} tabIndex={-1} {...other}>
<ButtonBase
component="span"
className={className}
onClick={onClick}
ref={ref}
tabIndex={null}
{...other}
>
{direction === 'left' ? <KeyboardArrowLeft /> : <KeyboardArrowRight />}
</ButtonBase>
);
Expand Down
4 changes: 2 additions & 2 deletions packages/material-ui/src/Tabs/TabScrollButton.test.js
View file
Open in desktop
Expand Up @@ -35,10 +35,10 @@ describe('<TabScrollButton />', () => {
});

describe('prop: !visible', () => {
it('should render as a div with root class', () => {
it('should render as a span with root class', () => {
const wrapper = shallow(<TabScrollButton {...props} visible={false} />);

assert.strictEqual(wrapper.name(), 'div');
assert.strictEqual(wrapper.name(), 'span');
assert.strictEqual(wrapper.hasClass(classes.root), true);
});
});
Expand Down
2 changes: 1 addition & 1 deletion packages/material-ui/src/Tabs/Tabs.test.js
View file
Open in desktop
Expand Up @@ -21,7 +21,7 @@ AccessibleTabScrollButton.propTypes = {
direction: PropTypes.string.isRequired,
};

const findScrollButton = (wrapper, direction) => wrapper.find(`button[aria-label="${direction}"]`);
const findScrollButton = (wrapper, direction) => wrapper.find(`span[aria-label="${direction}"]`);
Copy link
Member

Choose a reason for hiding this comment

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

That looks like it's making a11y worse. I stand by #15507 (review). We shouldn't set aria-labels for app authors. If anything we should look at how labels can be provided.

Copy link
Member

Choose a reason for hiding this comment

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

I have change the pull request, it doesn't set aria-labels for app authors. It makes the element disappear from the accessibility tools, they don't need it. How is it worse?
It's different in the test, the tests override the component to add an aria-label.

Copy link
Member

Choose a reason for hiding this comment

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

a11y is not just for screen readers. non-impaired users make use of these features as well. This looks like it's just hiding an a11y issue instead of solving it.

Copy link
Member

Choose a reason for hiding this comment

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

Do you have resources to support that the scroll buttons shouldn't be "hidden"? The only one I could find is how Antd, Vuetify and Element solve the problem.
I think that we should focus our energy on #6955.

const hasLeftScrollButton = wrapper => findScrollButton(wrapper, 'left').exists();
const hasRightScrollButton = wrapper => findScrollButton(wrapper, 'right').exists();

Expand Down