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

[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

merged 4 commits into from
May 12, 2019

Conversation

elnikolinho
Copy link
Contributor

@elnikolinho elnikolinho commented May 10, 2019

Closes #15371

Adds aria-label to TabScrollButtons, fixing an issue where accessibility tools pickup the buttons for not having discernible text

[Tabs] scrollButtons have an empty button error in compliance tools #15371
@mui-pr-bot
Copy link

mui-pr-bot commented May 10, 2019

Details of bundle changes.

Comparing: 048c9ce...de2ce8d

bundle parsed diff gzip diff prev parsed current parsed prev gzip current gzip
@material-ui/core +0.01% 🔺 +0.01% 🔺 317,107 317,135 86,643 86,651
@material-ui/core/Paper 0.00% 0.00% 67,869 67,869 20,172 20,172
@material-ui/core/Paper.esm 0.00% 0.00% 61,152 61,152 18,956 18,956
@material-ui/core/Popper 0.00% 0.00% 28,738 28,738 10,349 10,349
@material-ui/core/Textarea 0.00% 0.00% 5,513 5,513 2,384 2,384
@material-ui/core/TrapFocus 0.00% 0.00% 3,744 3,744 1,580 1,580
@material-ui/core/styles/createMuiTheme 0.00% 0.00% 15,960 15,960 5,779 5,779
@material-ui/core/useMediaQuery 0.00% 0.00% 2,106 2,106 974 974
@material-ui/lab 0.00% 0.00% 140,485 140,485 42,849 42,849
@material-ui/styles 0.00% 0.00% 51,354 51,354 15,189 15,189
@material-ui/system 0.00% 0.00% 14,458 14,458 4,175 4,175
Button 0.00% 0.00% 85,782 85,782 25,770 25,770
Modal 0.00% 0.00% 20,342 20,342 6,696 6,696
colorManipulator 0.00% 0.00% 3,904 3,904 1,543 1,543
docs.landing 0.00% 0.00% 51,215 51,215 11,373 11,373
docs.main 0.00% 0.00% 654,517 654,517 205,342 205,342
packages/material-ui/build/umd/material-ui.production.min.js +0.01% 🔺 +0.01% 🔺 296,014 296,042 84,043 84,052

Generated by 🚫 dangerJS against de2ce8d

@eps1lon eps1lon self-requested a review May 10, 2019 08:34
@oliviertassinari oliviertassinari added the component: tabs This is the name of the generic UI component, not the React module! label May 10, 2019
@oliviertassinari
Copy link
Member

oliviertassinari commented May 10, 2019

@elnikolinho A previous effort was done #15507. We couldn't reach a good solution.

This time, I have looked at how Ant Design, Vuetify and Element solve the problem, I have reproduced the same strategy.

@oliviertassinari oliviertassinari changed the title [Tabs] scrollButtons have an empty button error in compliance tools #15371 [Tabs] scrollButtons have an empty button error in compliance tools May 10, 2019
@@ -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

@oliviertassinari oliviertassinari May 10, 2019

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

@oliviertassinari oliviertassinari May 10, 2019

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.

@oliviertassinari oliviertassinari dismissed eps1lon’s stale review May 10, 2019 11:09

I believe you missed the update I have done to the original code. Could you look a second time? :)

@oliviertassinari
Copy link
Member

Hold on, I want to try something even cleaner.

@elnikolinho
Copy link
Contributor Author

Thanks!

@elnikolinho elnikolinho deleted the TabScrollButton-Accessibility branch May 12, 2019 23:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accessibility a11y component: tabs This is the name of the generic UI component, not the React module!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Tabs] scrollButtons have an empty button error in compliance tools
4 participants