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] Fix aria-label issue on the demos #15507

Merged
merged 3 commits into from Apr 29, 2019
Merged

[Tabs] Fix aria-label issue on the demos #15507

merged 3 commits into from Apr 29, 2019

Conversation

amangalvedhekar
Copy link
Contributor

@amangalvedhekar amangalvedhekar commented Apr 27, 2019 •
edited by oliviertassinari

Help with #15371

@mui-pr-bot
Copy link

mui-pr-bot commented Apr 27, 2019 •
edited

Details of bundle changes.

Comparing: 72f4f9f...f0b84b8

bundle parsed diff gzip diff prev parsed current parsed prev gzip current gzip
@material-ui/core -0.01% -0.01% 311,943 311,915 84,470 84,460
@material-ui/core/Paper 0.00% 0.00% 67,623 67,623 20,128 20,128
@material-ui/core/Paper.esm 0.00% 0.00% 60,988 60,988 19,024 19,024
@material-ui/core/Popper 0.00% 0.00% 31,114 31,114 10,803 10,803
@material-ui/core/Textarea -0.07% -0.13% 5,472 5,468 2,368 2,365
@material-ui/core/TrapFocus 0.00% 0.00% 3,731 3,731 1,565 1,565
@material-ui/core/styles/createMuiTheme 0.00% 0.00% 15,943 15,943 5,777 5,777
@material-ui/core/useMediaQuery 0.00% 0.00% 2,106 2,106 974 974
@material-ui/lab -0.00% -0.01% 141,033 141,029 42,668 42,662
@material-ui/styles 0.00% 0.00% 51,151 51,151 15,155 15,155
@material-ui/system 0.00% 0.00% 11,765 11,765 3,923 3,923
Button -0.00% -0.01% 85,937 85,933 25,747 25,744
Modal -0.02% -0.02% 20,579 20,575 6,609 6,608
colorManipulator 0.00% 0.00% 3,904 3,904 1,543 1,543
docs.landing 0.00% 0.00% 51,154 51,154 11,250 11,250
docs.main -0.00% -0.01% 649,336 649,318 202,481 202,468
packages/material-ui/build/umd/material-ui.production.min.js -0.01% -0.01% 293,659 293,631 82,420 82,414

Generated by 🚫 dangerJS against f0b84b8

eps1lon
eps1lon previously requested changes Apr 27, 2019
View reviewed changes
Copy link
Member

@eps1lon eps1lon left a comment

Choose a reason for hiding this comment

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

This is something that should be done by the app dev. Either mark aria-label as required in the propTypes or integrate with eslint-plugin-a11y. But using an enum seems like a bad idea. Especially since it assumes English.

@eps1lon eps1lon added accessibility a11y component: tabs This is the name of the generic UI component, not the React module! labels Apr 27, 2019
@oliviertassinari
Copy link
Member

oliviertassinari commented Apr 27, 2019 •
edited

@eps1lon The tabs action buttons are unreachable, I have only proposed this change so Wave does not report any issue on our documentation. Either we move forward with this change or we wait for Wave to handle this edge case.

I have a better idea :)

@oliviertassinari oliviertassinari self-assigned this Apr 27, 2019
@oliviertassinari
Copy link
Member

oliviertassinari commented Apr 27, 2019 •
edited

0 errors

Capture d’écran 2019-04-27 à 10 46 17

oliviertassinari
oliviertassinari reviewed Apr 27, 2019
View reviewed changes
packages/material-ui/src/Tabs/Tabs.js Outdated
@@ -514,6 +522,8 @@ Tabs.defaultProps = {
component: 'div',
indicatorColor: 'secondary',
ScrollButtonComponent: TabScrollButton,
scrollButtonLabel: position =>
Copy link
Member

Choose a reason for hiding this comment

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

@eps1lon Ok, you can review. I'm wondering if it's not overkilling. The more pragmatic approach I can think of is to set component to div and to add aria-hidden on the scrolll buttons. The only problem is that VoiceOver announces "button" when clicking on the button, I would be happy with this tradeoff. What do you prefer?

Copy link
Member

Choose a reason for hiding this comment

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

I'm wondering more about aria-hidden not working. Could you elaborate what "not working" means in this context? I thought screen readers ignore these elements.

Copy link
Member

Choose a reason for hiding this comment

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

Sure, adding the aria-label attribute doesn't change the Wave error report as well as the fact the VoiceOver announces "button' when clicking on the scroll buttons. I do think that Wave is designed to improve the VoiceOver behavior. IMHO having "button" announced when clicking on the scroll buttons (only accessible with a pointer device, is acceptable).

@oliviertassinari
Copy link
Member

oliviertassinari commented Apr 29, 2019 •
edited

@amangalvedhekar I would like to merge the aria-label fixes to start with :). I'm reverting the changes that are controversial. We need to resolve the discussion in #15507 (comment) before we can consider #15371 closed. Do you have a point of view on the concern?

@oliviertassinari oliviertassinari changed the title [Tabs] Update with aria-label attribute [Tabs] Fix aria-label issue on the demos Apr 29, 2019
oliviertassinari
oliviertassinari reviewed Apr 29, 2019
View reviewed changes
docs/src/modules/components/AppContent.js
@@ -36,7 +36,7 @@ function AppContent(props) {
<Container
component="main"
id="main-content"
tabIndex="-1"
tabIndex={-1}
Copy link
Member

Choose a reason for hiding this comment

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

Because 1:

@types/react/index.d.ts

interface HTMLAttributes<T> extends DOMAttributes<T> {
  …
  tabIndex?: number;

And 2: we use a number most of the time: consistency!

oliviertassinari
oliviertassinari reviewed Apr 29, 2019
View reviewed changes
packages/material-ui/src/Backdrop/Backdrop.js
@@ -39,7 +39,7 @@ const Backdrop = React.forwardRef(function Backdrop(props, ref) {
},
className,
)}
aria-hidden="true"
aria-hidden
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 looked at it on reach-ui side, they use both interchangeably. How does it work?

aria-hidden -> shorthand notation expension aria-hidden={true} -> DOM string casting aria-hidden="true"

Copy link
Member

Choose a reason for hiding this comment

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

There's a special terminology for this in the DOM spec but in the end these types of attributes just need to exist. No need to pass a value.

@oliviertassinari oliviertassinari merged commit f1e89c3 into mui:next Apr 29, 2019
@amangalvedhekar amangalvedhekar deleted the button-error-accessibility branch April 29, 2019 12:15
@oliviertassinari oliviertassinari mentioned this pull request May 10, 2019
Merged
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oliviertassinari oliviertassinari oliviertassinari left review comments

@eps1lon eps1lon eps1lon left review comments

Assignees

@oliviertassinari oliviertassinari

Labels
accessibility a11y component: tabs This is the name of the generic UI component, not the React module!
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@amangalvedhekar @mui-pr-bot @oliviertassinari @eps1lon