Add missing label to border radius range control. #42118
Conversation
afercia
added
[Package] Components
|
Size Change: +780 B (0%) Total Size: 1.25 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Thank you for your contribution, @afercia !
The improvements to the RangeControl component that you're proposing are actually going to be added by #40535 and #41444 (very soon, hopefully in a couple of days!).
If you don't mind, I'd rather keep this PR focused on making changes only to BorderRadiusControl — would you mind removing the changes to RangeControl ?
Otherwise, with respect to the changes to BorderRadiusControl, I have a few questions:
- it looks like the
aria-labelmay be enough to label this input field (source) - in case it's not enough, I wonder if we should also:
- look into adding a
labelto theinput[type='number]rendered alongside the slider input - look into adding the same labels to the four
input[type='number]that are rendered when the border radii are un-linked
- look into adding a
cc'ing @alexstine in case he has any inputs on the label vs aria-label attributes, and @aaronrobertshaw (since he has experience working on both BorderRadiusControl and RangeControl)
|
It is always better to have the HTML label. However, the aria-label works when providing the HTML label is simply not practical and the amount of work to add it would be too much of an undertaking. TLDR: Use the aria-label only when HTML label takes 2X+ the work. |
Thanks for the ping. As this PR shows, there's not much involved in getting an HTML label in place and still hidden from view for the range input. Is there any value in having all the approaches to labelling within the component consistent? That is, the
The
Would another option to getting a meaningful HTML label in place be to replace the
Narrowing the scope of this PR to exclude the RangeControl changes that are already covered elsewhere sounds good to me. |
That is the main question that I'm asking — I don't have any issues with the addition of a |
|
Removed the changes to Re: best practices for labelling, we should keep into consideration a wide range of user needs and not focus only on screen reader users needs. Worth also mentioning that screen readers are used also by low vision users. This PR isn't the best place for detailed explanations 🙂 Very shortly: A visible When hiding labels Note about the usage of 'Top left, incrementable edit text number field, Radius' Without the legend, it would only announce: 'Top left, incrementable edit text number field' Users wouldn't have a clue what 'Top left' is about and they would be forced to explore the surrounding text to figure it out. I'd strongly recommend to reuse |
There was a problem hiding this comment.
Code changes LGTM and test well as per instructions 🚀
Thank you for working on this improvement, @afercia , and for taking the time to give an overview of how label, aria-label, tooltips, fieldset and legend all affect the accessibility tree in different ways 🙇
Fixes #42098
What?
Adds a missing label to the range input in the Border radius control.
Why?
All form controls need to be labelled.
How?
labelprop with meaningful valuehideLabelFromVisionprop to make the label visually hiddenhideLabelFromVisionwas added in a previous PR but it was undocumentedTesting Instructions
<label>element associated with aforattribute to its IDaria-labelattribute with the same text of the<label>elementOptionally:
npm run storybook:devhideLabelFromVisionprop toggles the label visibilityNote: using both a
<label>element and anaria-labelattribute is redundant. The current component just works this way and a refactoring is out of the scope of this PR.Screenshots or screencast