fix(Tabs): add aria-label to overflow scroll buttons #7353
Conversation
|
✔️ Deploy preview for carbon-elements ready! 🔨 Explore the source changes: c574d09 🔍 Inspect the deploy logs: https://app.netlify.com/sites/carbon-elements/deploys/60070f7c31b9ce000776a930 😎 Browse the preview: https://deploy-preview-7353--carbon-elements.netlify.app |
|
✔️ Deploy preview for carbon-components-react ready! 🔨 Explore the source changes: c574d09 🔍 Inspect the deploy logs: https://app.netlify.com/sites/carbon-components-react/deploys/60070f7c0d56650007957a7b 😎 Browse the preview: https://deploy-preview-7353--carbon-components-react.netlify.app |
| /** | ||
| * Provide the props that describe the left overflow button | ||
| */ | ||
| leftOverflowButtonProps: PropTypes.object, |
There was a problem hiding this comment.
Is there a way we could allow folks to configure the labels without giving them a way to pass in any prop to the buttons? Maybe something like overflowStartLabel, overflowEndLabel? or scrollPrevLabel, scrollMoreLabel (just throwing out some ideas)
Exposing that this is a button and allowing folks to configure it might expose some internal implementation details, or prevent us from refactoring like if we wanted the label to be inside of the button instead of aria-label, or would want to connect it using aria-labelledby or something 🤔
There was a problem hiding this comment.
yeah I was looking ahead and figuring that people would want to extend the button so I didn't want to make the prop too restrictive by making it label-only
the use of aria-label was suggested by @carmacleod since there is no visible label for these overflow buttons
There was a problem hiding this comment.
If we're anticipating folks to configure the buttons directly it might be helpful to extract them as components and allow folks to pass props directly to them (and support providing these buttons as children)
Definitely think aria-label is the right call, just was saying that this would restrict changes we make if we ever want to change how these buttons are implemented (e.g. if we wanted to use aria-labelledby for some reason).
An example of this could be the components that forward props to downshift, for example. It makes updating that package difficult as we have to manage the breaking changes in props (like name changes) if we want to update it in a minor release.
There was a problem hiding this comment.
I just don't know that we need to provide that much configurability for these buttons when the logic that displays them is all internal. is this a future change that you are predicting for this component? it seems like we're solving an issue that isn't occurring given the component spec and a11y requirements
emyarod
force-pushed
the
7304-tab-overflow-button-labels
branch
2 times, most recently
from
0467b02
to
f7e0918
Compare
emyarod
force-pushed
the
7304-tab-overflow-button-labels
branch
from
f7e0918
to
c933ff7
Compare
|
Will this fix make it into the upcoming release? |
|
unfortunately not the initial release, but it is still awaiting reviews so it still may be released in a patch |
emyarod
added
status: ready to merge 🎉
and removed
status: one more review 👀
status: ready for review 👀
labels
Closes #7304
This PR sets a default aria-label to the scroll overflow buttons to remove a11y violations. Since the buttons are not in the tab order a visible label is probably not necessary. Added support for additional props on the scroll overflow buttons to allow for different
aria-labelvaluesChangelog
New
aria-labelon scroll overflow buttonsTesting / Reviewing
Confirm that the a11y checker violation on the overflow button labels is no longer raised