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

fix(react): use slider label for slider and input by default in V11 #10252

Merged
merged 5 commits into from
Dec 9, 2021
Merged

fix(react): use slider label for slider and input by default in V11 #10252

merged 5 commits into from
Dec 9, 2021

Conversation

abbeyhrt
Copy link
Contributor

@abbeyhrt abbeyhrt commented Dec 7, 2021

Closes #10172

Updates Slider label to be for the input by default and removes the default prop for ariaLabelInput in v11. I could be wrong but it felt weird to have an aria label and a label for the input so that's why I removed the default prop value.

Question, should labelText be required since it's automatically used to label the slider and now the input potentially? I know users could still provide an aria label for the input but without labelText the slider has no associated label.

Changelog

Changed

  • Slider label now optionally labels the input

Testing / Reviewing

Go to the storybook Slider's playground story and remove ariaLabelInput and verify that the for from label has the same id as the input.

@abbeyhrt abbeyhrt requested a review from a team as a code owner December 7, 2021 21:24
@abbeyhrt abbeyhrt changed the title fix(react): slider label fix(react): use slider label for slider and input by default in V11 Dec 7, 2021
@netlify
Copy link

netlify bot commented Dec 7, 2021

✔️ Deploy Preview for carbon-react-next ready!

🔨 Explore the source changes: 3b19d44

🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-react-next/deploys/61b255696178e500086bf43d

😎 Browse the preview: https://deploy-preview-10252--carbon-react-next.netlify.app

@netlify
Copy link

netlify bot commented Dec 7, 2021

✔️ Deploy Preview for carbon-elements ready!

🔨 Explore the source changes: 3b19d44

🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-elements/deploys/61b255692012a80007b92735

😎 Browse the preview: https://deploy-preview-10252--carbon-elements.netlify.app

@netlify
Copy link

netlify bot commented Dec 7, 2021

✔️ Deploy Preview for carbon-components-react ready!

🔨 Explore the source changes: 3b19d44

🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-components-react/deploys/61b25569f43791000882f9d0

😎 Browse the preview: https://deploy-preview-10252--carbon-components-react.netlify.app

@tay1orjones
Copy link
Member

tay1orjones commented Dec 8, 2021

should labelText be required

@abbeyhrt That might be more a question for @carbon-design-system/design. I'd lean towards no. If it's required, users won't be able to configure a slider without a visible label.

It's one of those odd situations where the props are sort of conditionally required based on one another. If there's no labelText, ideally ariaLabelInput would be required (or have a default). That might be a reason to leave in the default for ariaLabelInput? Also would it make sense to apply aria-label={ariaLabelInput ? ariaLabelInput : null} to the slider if there's no labelText?

@abbeyhrt
Copy link
Contributor Author

abbeyhrt commented Dec 8, 2021

@tay1orjones I see what you mean and I think I would prefer to keep the default for ariaLabelInput too but then {ariaLabelInput ? ariaLabelInput : null} would just always be the default unless someone explicitly says ariaLabelInput='', right?

Thinking about what you said, I’m kind of not sure if I agree with Michael Gower’s original point in the thread that kicked off the issue. I think it would make sense for the label to label the slider and input if labelText was required, but since that is optional, it feels strange to me to rely on it in two places. Especially if that means not having a default for ariaLabelInput. To me, there are two tracks: the label for the input and the label for the slider and it seems okay to me if they stay separate like they are now

Copy link
Member

@tay1orjones tay1orjones left a comment

Choose a reason for hiding this comment

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

@abbeyhrt Yeah, I agree with what you're saying. Especially about there being two tracks. 1. labelling the slider and 2. labelling the input.

Would it be best if we could cover both together, but also enable each to be individually labelled as well in the event labelText isn't provided? If so, this is most of the way there:

  • Cover both tracks by leaving labelText optional and have it linked to both the slider and the input
  • Cover track 1 by providing the option to label the slider via a new prop ariaLabelSlider (with no default)
  • Cover track 2 by providing the option to label the input via the existing prop ariaLabelInput (with no default)

Copy link
Member

@tay1orjones tay1orjones left a comment

Choose a reason for hiding this comment

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

@abbeyhrt Worth stating that despite my thoughts above, I'm also totally fine merging this as is!

The primary feedback from Michael is covered by you adding htmlFor={`${id}-input-for-slider`} to the label 👍

Copy link
Contributor

@dakahn dakahn left a comment

Choose a reason for hiding this comment

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

Tested with NVDA and JAWS latest on Windows 11

Nice work! I can detect no discernable difference between our previous implementation and this one, except that with NVDA the number input reads the Slider label whereas before it did not. Which is awesome 👍🏾 🏄🏾

@abbeyhrt
Copy link
Contributor Author

abbeyhrt commented Dec 9, 2021

Okay, I think i'm going to leave this as is for now and merge it in. Slider still needs to be refactored to a functional component so we might be able to revisit this then.

@abbeyhrt abbeyhrt merged commit a69b00e into carbon-design-system:main Dec 9, 2021
@mbgower
Copy link

mbgower commented Jan 12, 2022

I think it would make sense for the label to label the slider and input if labelText was required, but since that is optional, it feels strange to me to rely on it in two places. Especially if that means not having a default for ariaLabelInput. To me, there are two tracks: the label for the input and the label for the slider and it seems okay to me if they stay separate like they are now

@abbeyhrt and @tay1orjones Thanks for implementing a change for this issue!

I just wanted to respond to this comment. The accessibility challenge here is to try to reduce the effort to make a consistently named slider and input. If the designer must always annotate the name of the slider input in addition to naming the slider, there's more work. If the developer must always implement the slider input name, there's more work. There's also obviously more chance for the name to be overlooked. At that point, you end up with one or both of the controls inheriting whatever generic label is in the code. The input is "Label for slider value" which I think we can agree is not great. (At the least shouldn't it be improved to something like "slider value"?)

It might be easier to think of the construct this way: the slider must have an accessible name.

One of two things must occur. Either the slider has a (properly constructed) visible label or it has an accessible name (usually set by aria). The input for the slider is optional, but if it is there, it never has a visible label. If it always takes whatever name is specified for the slider, it's tough for an implementer to get it wrong -- and it takes less effort for the designer and dev.

When you do refactor the slider component, ideally the code can be constructed such that this always comes about.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[a11y]: slider label is not used for slider input label
4 participants