-
-
Notifications
You must be signed in to change notification settings - Fork 32.2k
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] Fix aria-label issue on the demos #15507
[Tabs] Fix aria-label issue on the demos #15507
Conversation
Details of bundle changes.Comparing: 72f4f9f...f0b84b8
|
There was a problem hiding this 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.
@@ -514,6 +522,8 @@ Tabs.defaultProps = { | |||
component: 'div', | |||
indicatorColor: 'secondary', | |||
ScrollButtonComponent: TabScrollButton, | |||
scrollButtonLabel: position => |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
@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? |
@@ -36,7 +36,7 @@ function AppContent(props) { | |||
<Container | |||
component="main" | |||
id="main-content" | |||
tabIndex="-1" | |||
tabIndex={-1} |
There was a problem hiding this comment.
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!
@@ -39,7 +39,7 @@ const Backdrop = React.forwardRef(function Backdrop(props, ref) { | |||
}, | |||
className, | |||
)} | |||
aria-hidden="true" | |||
aria-hidden |
There was a problem hiding this comment.
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"
There was a problem hiding this comment.
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.
Help with #15371