BorderBoxControl: Improve accessibility and labelling #42351
Conversation
aaronrobertshaw
added
[Type] Enhancement
aaronrobertshaw
changed the title
|
Thank you for this PR @aaronrobertshaw! cc @afercia since you were checking these labels last week 👍 |
|
Size Change: +4 B (0%) Total Size: 1.26 MB
ℹ️ View Unchanged
|
aaronrobertshaw
force-pushed
the
update/border-box-control-labelling
branch
from
544683d
to
ba8a734
Compare
There was a problem hiding this comment.
I wonder if, given the recent changes to BorderControl, is it going to be ok to have nested fieldsets. I have looked around for answers, and only found a few pointers.
It is possible to nest one
fieldsetelement inside another, but it is not recommended. Screen readers do not automatically indicate the end of thefieldsetelement, so it is impossible for screen reader users to confidently know which fields belong within which fieldset.
Source https://accessibility.blog.gov.uk/2016/07/22/using-the-fieldset-and-legend-elements/
Also found a few interesting related comments in https://www.tpgi.com/fieldsets-legends-and-screen-readers/
An alternative approach, according to the official W3C's guidelines, would be to use a div role=group and the aria-labelledby attribute — @afercia , do you have any insights?
aaronrobertshaw
force-pushed
the
update/border-box-control-labelling
branch
from
ba8a734
to
cd7ee29
Compare
|
Thanks for the link to the alternate approach in the W3c Guidelines @ciampo 🙇 I've updated this PR to adopt the |
|
It would be great if @afercia could help us validate this new approach. If the approach is validated, we could even consider using this approach for BorderControl, to avoid any potential nested fieldset/legend in higher level components? |
|
Thanks for working on this and for the ping. The post on accessibility.blog.gov.uk is a very good and informative source. It's the page I usually point people to for fieldset and legend related questions. However, the statement:
isn't entirely accurate. Actually, some screen readers do announce the end of a fieldset, some don't. I'm assuming that statement mostly refers to the case of nested fieldsets.
Not surprisingly, the way screen readers announce this pattern may be slightly different from the way they announce a HTML fieldset + legend. In my testing, some screen readers repeat the legend when focusing each input field within the fieldset. They don't do this with the ARIA pattern. While this would be a minor inconsistency, the golden rule we should follow is to always use HTML features when possible and avoid unnecessary ARIA. See the First Rule of ARIA Use. I haven't looked at the code so I'm not sure why there would be nested fieldsets but if possible I'd recommend to make a little effort to stick with HTML features and use only one fieldset + legend. |
In the context of #42095, we've been working on making improvements to both Such improvements have already been applied to The thing is, Given the extra context, do you have any advice on what the best solution would be? |
|
Thanks for working on this!
Yeah, a quick test of the storybook components on this branch with VoiceOver shows the label for the Could we perhaps make BorderControl render a fieldset by default, but allow it to render as something else with a prop? That would allow us to wrap BorderBoxControl in a fieldset with a legend, while wrapping the BorderControl inside it in a plain div. Maybe just a boolean prop that removes the semantic wrapper and the legend, allowing the form fields to be used as part of a larger semantic grouping? Unrelated to this PR, but the "Border color and style picker" button is extremely verbose when non-default styles are selected. Are there already planned improvements to that or should I open an issue? |
|
@ciampo thanks for the clarification. Looking a bit into the code, I'm not sure adding the fieldset + legend to
That's a bit too much 🙂 Ideally, there should be only one fieldset/legend wrapping all the 4 controls. I'd tend to think adding the fieldset/legend to the Is Aside: does anyone know what the |
I'll let other folks who are more knowledgeable about how
No planned improvements, AFAIK (cc'ing @aaronrobertshaw )
From the component's README: |
At present, the Unfortunately, I ran out of time today to update both this PR and the
I'd be happy to change it to anything that is easier on the ear or eyes. I'm open to any suggestions that might strike the right balance between clarity of purpose and brevity. ⚖️ |
This reverts commit cdf6a48.
aaronrobertshaw
force-pushed
the
update/border-box-control-labelling
branch
from
cd7ee29
to
d948dbd
Compare
|
Thanks everyone for all the feedback on this. 🙇 Given the As for the |
|
Sounds good! Let's focus on #42611 before getting back to this PR |
Related:
What?
Updates the
BorderBoxControlcomponent to improve labelling, DOM Structure etc.Why?
Accessibility is important.
How?
fieldsetand its primary label to alegendgetByRole()in the unit tests to also filter inputs by name.Testing Instructions
npm run test-unit /packages/components/src/border-box-control/testBorderBoxControlcontinues to function correctly within the Storybookfieldsetwith an appropriatelegendas the label.