[EuiFormRow] API and a11y cleanup

[myasonik]

Proposal

Deprecate labelType and hasChildLabel on <EuiFormRow> and, instead, programmatically detect what the correct element should be.

Our current solution requires the developer to know ahead of time what's going to render and how the child elements effect what you have to pass into EuiFormRow.

Details

In general, it should be <label> unless the child form elements already have an accessible name wired up.

So, elements that need a label: <input>, <meter>, <output>, <progress>, <select>, and <textarea>. (Technically, <button> supports having a <label> wired up but it doesn't work very well.)

And, elements can be labelled with a wired up <label>, aria-label, or aria-labelledby.

Extra adjacent fixes

• [ ] <legend> needs to be the first child of <fieldset> (#3032)

[myasonik]

CC @thompsongl

[cchaos]

I agree that there is quite a large separation of concerns when it comes to consumers using EuiFormRow and form elements. It seems to me that if we really want to take on an endeavor of making this form creation experience better, we shouldn't rely on consumers understanding how to build forms.

Caroline's proposal

No longer force consumers to wrap form elements in EuiFormRows. Instead, allow them to pass labels, helpText and such directly to the form elements like EuiFieldText.

Today's example

<EuiFormRow 
  label="Text field" 
  helpText="I am some friendly help text.">
  <EuiFieldText name="first" />
</EuiFormRow>

Proposed example

<EuiFieldText 
  name="first" 
  label="Text field" 
  helpText="I am some friendly help text." 
/>

Then the EuiFieldText can decide how to best handle the label and help text.

The form element component

The component itself can still use EuiFormRow inside of it, but can pass down the correct parameters necessary. The component can then also enforce accessibility concerns based particularly on that type of input.

Phasing

This would mean a major break to sooooo many layouts that it's really quite a scary endeavor. However, I think it can be phased by creating secondary components to the inputs like EuiFieldTextRow and then we can slowly turn over and phase out the manual EuiFormRow approach.

[chandlerprall]

I like the idea of having each EuiField* component able to receive label & helpText, and automatically inject a form row when present. I imagine there are use cases where it makes sense to explicitly use the nested <EuiFormRow><EuiFieldText></></>, and we should take care not to block those.

[cchaos]

Yeah that's why I'm thinking the best approach might be to add a new component of EuiField*Row that will do all the thinking for them.

[myasonik]

I'd really like to phase out EuiFormRow+<EUI form element> as the primary way to create forms so I'm very much onboard with this plan.

Even if we ignore some of the cases where it would be tricky to create a single element and only ship the easy ones, that would already reduce the surface area for a11y bugs that implementing developers have to wade through.

[myasonik]

🤔 Though, even if we don't ship a new component for every possible case, we should probably come up with a holistic plan for all form component so we don't ship something we regret starting later...

[snide]

Beware, old man practicality appears in the ring!

I just want to throw a little caution into this thread. This is a much easier problem to solve at a component level (and it will still be hard), then it is to implement across lots of applications already in flight. We now need to worry about EUI beyond even Cloud and Kibana and I'm pretty sure the upgrade cost, even in a phased approach, would be high. We need to consider the overall gains we'd get from this kind of change (right now: an improved developer experience that would lead to more consistent a11y usage) before we committed it to the roadmap. I've never known accessibility work to just magically work for all scenarios, and we might find we replace one magical solution that sometimes breaks with another magical solution that sometimes breaks a little less. I don't want to discourage the discussion here, but it's just something to keep in mind.

That said, I think having the props on the elements themselves sounds elegant, but I bet there's a lot of situations we're not thinking about and I think it would take a long time to get rid of EuiFormRow.

My advice would be to wait a couple months and see if we still feel this solution makes sense later (considering this ticket originally wanted to remove something we also recently added). This isn't a hard-break problem atm and there are plenty of ways out of the box currently (the hasLabel props, the separated components...etc). This would also be a good place to possibly do some outside research and see how others are doing it. Just as an example, I AtlasKit failing in very similar ways to the problems we have, using very similar methods.

[myasonik]

I've been taking note of what other libraries do and here's my table so far:

Design system	Does what we do	Does Caroline's proposal	Does something else
Google's Material-UI	✅
ant-design		✅
Salesforce's Lightening		✅
IBM's Carbon			Splits the difference a little but more like Caroline's proposal
GOV.UK		✅
Mixpanel		✅
Uber's Base			Supports both
Thumbtack's Thumbprint	✅
Github's Primer	✅
Zendesk's Garden	✅
Microsoft's Fabric			Supports both
Meetup's Swarm		✅

So it seems like there's not a clear answer coming.

Given the improved consistency and accessibility that we can enforce with Caroline's proposal, but the enormous deprecation struggle it would be to remove EuiFormRow, I think supporting both is probably the best way forward.

Individual components could be the recommended way forward while EuiFormRow and the other atomic pieces would be the building blocks that would have continued support for the immediate future but could de-emphasized in the docs.

[myasonik]

Talked to @cchaos about this and saw #3037 in the same day so I'm going to try to get a POC up for this soon so there's something more concrete to talk about

[cchaos]

I got burnt out on Emotion, so here's a POC I've been meaning to cook up https://github.com/elastic/eui/pull/3837

[thompsongl]

👀

So work on #5157 (and all combobox/EuiSelectable rebuilds more broadly) has surfaced for me how EuiFormRow makes it difficult to use and propagate id attributes. For instance, per ARIA spec, we need to reference the id of the <label> (provided by EuiFormRow) on at least two combobox/listbox elements with aria-labelledby, but don't have access. Relatedly, the child component doesn't know if it has a <label> with a for attribute in reference.

The problem is that we can't just prop-drill a labelId prop because we don't prescribe what can be children; basic HTML form elements are acceptable. This would cause all kinds of "invalid attribute" console errors.

I'm mostly just noting the problem here, but one thought is to use React context to make each EuiFormRow a context provider that then any component can opt into consuming configuration data depending on its need.

[cchaos]

We just really need to remove the idea that consumers need to wrap inputs with EuiFormRow altogether. Each input should accept stuff like label and helpText. Then it can provide all the hook ups it needs. I intermittently work on PR #3837, but it's close just a couple oddities left I think around prepend/append.

[github-actions[bot]]

👋 Hi there - this issue hasn't had any activity in 6 months. If the EUI team has not explicitly expressed that this is something on our roadmap, it's unlikely that we'll pick this issue up. We would sincerely appreciate a PR/community contribution if this is something that matters to you! If not, and there is no further activity on this issue for another 6 months (i.e. it's stale for over a year), the issue will be auto-closed.

[github-actions[bot]]

❌ Per our previous message, this issue is auto-closing after having been open and inactive for a year. If you strongly feel this is still a high-priority issue, or are interested in contributing, please leave a comment or open a new issue linking to this one for context.

[cee-chen]

Re-opening - this is still valid and on my "would like to do" list sometime this year