Make EuiDescribedFormGroup title accessible

[elizabetdev]

Summary

Closes #2888

Previously, it was decided on #2783 to introduce a <fieldset /> to group elements in the EuiDescribedFormGroup component. Because the first element inside the fieldset must be a <legend /> it was decided to:

• Wrap the legend with an <EuiScreenReaderOnly> and add an aria-hidden="true" to the <EuiTitle />

This decision created some issues described in #2888. For instance in Kibana Advanced Settings, users could no longer navigate by heading to any setting.

Fix

To fix this issue I decided to:

• Replace the <fieldset /> with a <div role="group" />. Actually, before the change introduced on Described form group a11y #2783 the component was already like that.
• This way we don't need the aria-hidden on the<EuiTitle /> element and we no longer need a <legend />.

Checklist

• [ ] Check against all themes for compatibility in both light and dark modes
• [ ] Checked in mobile
• [ ] Checked in IE11 and Firefox
• Props have proper autodocs
• [ ] Added documentation examples
• Added or updated jest tests
• Checked for breaking changes and labeled appropriately
• Checked for accessibility including keyboard-only and screenreader modes
• A changelog entry exists and is marked appropriately

[kibanamachine]

Preview documentation changes for this PR: https://eui.elastic.co/pr_2989/

[kibanamachine]

Preview documentation changes for this PR: https://eui.elastic.co/pr_2989/

[myasonik]

But... Now we don't have a <legend />...

To shim the expected functionality, we'd need to add an aria-describedby to each form element (e.g., <input />, <textarea />, etc.) rendered inside this group which seems a little tricky. Something that would have to happen after render, I think?

[elizabetdev]

In the past, we used to have an issue with having an aria-describedby. The description text was served as the help text for the input by using the aria-describedby when there was only one EuiFormRow .

This solution was making the screen readers read the text twice. I think if we use again aria-describedby on <input /> s or <textarea />s we will end up with the same issue. Unless we have an aria-hidden on the element with the id. That also causes another issue. The one we're trying to solve.

I think we should separate things. The title and description of the EuiDescribedFormGroup are used as a way to provide additional content. If users want to create a help text for each <input /> or <textarea /> they should use a EuiFormRow help-text prop or an aria-label on the form element.

But I'm open to other solutions. 🙂

@cchaos, @myasonik any suggestions?

[cchaos]

@myasonik Can we not just rely on DOM order and heading structure for screenreaders to read these types of pages. Essentially it renders:

<headingLevel />
<paragraph />
<inputLabel for="input" />
<input />

... repeat ...

[myasonik]

It depends...

That structure is really good for things like describing a form. For example, stuff like "This info will only be used for these specific purposes" or "These settings apply to a specific user space and not to others".

But it starts to break down when you try describing form fields. For example, not having a legend becomes really difficult is an address "field" (where there actually multiple fields for one "value"). An individual input might be labelled with "Line 1" or "State" and the legend provides the context that this is for a billing address or something.

I see how this is tricky to solve with our current setup though so if we want, we can ship this (it's probably better than clobbering semantics like we are right now) but I'd want to open a new issue to reintroduce <fieldset>/<legend> and probably tackle the whole lot of the form issues by making a plan forward for #2493.

[elizabetdev]

I think we should ship this to fix the #2888 bug and we open a new issue to reintroduce the <fieldset>/<legend>. If we all agree, I'm happy to merge.

[cchaos]

I agree, let's just fix the a11y issue and then come up with a solution in another issue so we know exactly what the path is forward.

[kibanamachine]

Preview documentation changes for this PR: https://eui.elastic.co/pr_2989/

[thompsongl]

LGTM assuming a follow-up issue