Described form group a11y#2783
Conversation
myasonik
left a comment
There was a problem hiding this comment.
Will this cause a breaking change in Kibana?
My comments are for further improvements but shouldn't block this PR. If they can't get into this PR, let's make a new issue for them.
Also, reading #762, I think there's a good suggestion in there to limit EuiTitle to only allow h1 - h6 children. If we're going to close that ticket, we should make a new one for that as well.
src/components/form/described_form_group/described_form_group.js
Outdated
Show resolved
Hide resolved
src/components/form/described_form_group/described_form_group.js
Outdated
Show resolved
Hide resolved
andreadelrio
left a comment
There was a problem hiding this comment.
I'll leave the review of the a11y stuff to Michael. Just added a couple of small suggestions.
Also, now that you're in that code maybe we can change the size here (title of the "Multiple fields" example) to s. Right now "Multiple Fields" kind of looks like it's another section (at 28px) and at least for me, since it was the first time looking at that page in our docs it was confusing
to this
Co-Authored-By: Andrea Del Rio <delrio.andre@gmail.com>
Co-Authored-By: Andrea Del Rio <delrio.andre@gmail.com>
snide
left a comment
There was a problem hiding this comment.
Agree with @myasonik. Fieldset feels better if possible. Removing the prop will make this a technical breaking change. When we're removed a prop like this we've generally flipped a coin as to whether we mark it as breaking since the downstream components would still work fine and at works will spit out a console error. This is used so commonly in Kibana though that it's probably best to mark it as such. When you get ready to merge this please write up the upgrade path in your PR description. I'd assume that should simply be "Remove the idAria prop...etc"
src/components/form/described_form_group/described_form_group.js
Outdated
Show resolved
Hide resolved
snide
left a comment
There was a problem hiding this comment.
LGTM. Did another review of the code and tested the output in a Screen reader.
|
|
||
| <EuiFlexGroup gutterSize={gutterSize}> | ||
| <EuiFlexItem> | ||
| <span ref={ref} title={innerText}> |
There was a problem hiding this comment.
@miukimiu Quick question, why is the title prop needed here?
There was a problem hiding this comment.
I copied the code from the InnerText snippet and I must have missed it. I can remove it on #2810 or create a different PR.
Summary
Closes #762
When using just one
EuiFormRowinside aEuiDescribedFormGrouptheEuiDescribedFormGroup descriptiontext was served as the help text for the input. This was causing the text to be read twice by screen readers.Once when users were reading the description and again when users are inside the input. This input was getting an
aria-describedbywith theidof theEuiDescribedFormGroup description.I think it's not a good idea to use the
EuiDescribedFormGroup description propas a way to display help text for the input. If users want to create a help text for the input they should use aEuiFormRow help-text propor anaria-labelon the form element.The
EuiDescribedFormGroup descriptionshould be used to provide an additional text and not as a help text for a form element.Breaking change
idAriaprop fromEuiDescribedFormGroupSuggested upgrade
idAriaprop usage on anyEuiDescribedFormGroupcomponents being used in your application.Checklist