Update HeightControl component to label inputs

[andrewhayward]

What?

Adds accessible names to both inputs in the HeightControl component, replacing the fieldset wrapper and using aria-controls instead.

Why?

Currently, HeightControl uses a <fieldset> to label the input fields, but this doesn't provide an accessible name for either. Both fields should have an appropriate name.

How?

• Updates the RangeControl component to accept an optional ID.
• Replaces the <fieldset>/<legend> combination with a <label>, pointing at the text input, to ensure it has an accessible name.
• The range input references the <label> with an aria-labelledby to get its own accessible name.
• The range input gains an aria-controls attribute, pointing at the text input (while acknowledging that aria-controls has little support at the time of writing).

Testing Instructions

• Launch storybook: npm run storybook:dev
• Go to the HeightControl component
• Check that both fields have accessible names (using the inspector)

Testing Instructions for Keyboard

Nothing has functionally changed, so both inputs should remain as expected

[github-actions]

👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @andrewhayward! In case you missed it, we'd love to have you join us in our Slack community, where we hold regularly weekly meetings open to anyone to coordinate with each other.

If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information.

[andrewhayward]

Potentially part of a larger body of work relating to #42630.

[alexstine]

👍 Let me try to get to this later today. Thanks.

[ciampo]

Hey @andrewhayward , thank you for opening this PR and working on what is definitely a complex task.

I'm going to recap the changes in this PR, mainly to confirm with you whether I'm understanding correctly what you're proposing, and to help other reviewers:

• HeightControl renders both a UnitControl and a RangeControl component, which are used as alternative forms of input to edit the same underlying value. The HeighControl component takes care of keeping UnitControl and RangeControl's values in sync.
• Currently on trunk, HeightControl uses a combination of fieldset / legend to label the input fields.
• Given that UnitControl and a RangeControl are alternative to each other, they fall in the category of what you have previously described as "Redundant Inputs".
• In this scenario, you're proposing of dropping the fieldset/legend approach, and instead:
  • labelling the "primary" input (ie.UnitControl) in a more traditional way (i.e with label element and the for attribute)
  • labelling the "redundant" input (ie.RangeControl) via the aria-labelledby attribute, and use the aria-controls attribute to signal that the redundant input controls the same value as the "primary" input.

Typically, I've seen the aria-controls attribute being used for composite widgets like comboboxes — this approach is definitely interesting, and one that we haven't explored yet! I'd be very curious to test it further across different browsers / OSs / screen readers. Have you already done some testing?

The most interesting and challenging aspect of this approach is in evaluating its scalability / composability. A few considerations:

• UnitControl itself renders multiple input elements, in a "composite" way (ie. the quantity and the unit inputs)
• RangeControl can render a redundant input field itself (when the withInputField prop is set to true)

Would this approach still work well when wrapping around (or nesting under) other similar composite and/or redundant components?

[ciampo]

Would it be better to allow for empty string as the value of isProp ?

Suggested change

	return idProp || id;
	return idProp ?? id;

[andrewhayward]

Would it be better to allow for empty string as the value of idProp ?

I'm not sure as I can think of a scenario where an empty string would be an acceptable value. Currently, the range control auto-generates its own ID, and always uses it. The change just means that it can be overridden, and if the range control expects a value for any reason, an empty string might cause confusion.

But this is not a strongly held opinion! I'd be happy with your suggested change if those more familiar with the code than me think it appropriate. I took it from a current pattern that exists in a few places already (to the point that it might be worth adding a useOverridableInstanceId function in @wordpress/compose!).

[ciampo]

Thank you, this makes sense to me!

[ciampo]

This way of generating IDs looks good. An alternative could be to simply use the recently introduced useId hook to generate three separate IDs? Although I'm definitely fine keeping the current approach, given it tests well.

[andrewhayward]

This way of generating IDs looks good. An alternative could be to simply use the recently introduced useId hook to generate three separate IDs? Although I'm definitely fine keeping the current approach, given it tests well.

I just took the useInstanceId pattern found elsewhere and extended it, to be honest, so I don't feel strongly either way. Perhaps worth noting though that even the useId docs say this about related elements:

If you need to give IDs to multiple related elements, you can call useId to generate a shared prefix for them

So even React's expectation seems to be that in this situation you'd generate one stem and create multiple IDs from it.

[ciampo]

That makes sense, thank you for the explanation

[ciampo]

Reading at the doc, it looks like BaseControl.VisualLabel is not meant to be used to render an actual label (even though it's possible to do so, through the as prop) — from the docs:

BaseControl.VisualLabel is used to render a purely visual label inside a BaseControl component.

It should only be used in cases where the children being rendered inside BaseControl are already accessibly labeled, e.g., a button, but we want an additional visual label for that section equivalent to the labels BaseControl would otherwise use if the label prop was passed.

We could take one of these approaches:

• keep the current solution (which means that we should probably update the BaseControl.VisualLabel docs?)
• render an additional <label /> element (and make it visually hidden). At the point, we could remove the as prop on BaseControl.VisualLabel and use it only for aesthetical reasons
• remove BaseControl.VisualLabel and replace it with a simple <label /> (although we'd need to somewhat style it).

Maybe @mirka has some more insights on what would be the better approach?

[mirka]

I'd like to see how we could enhance the BaseControl component itself so that it can support this kind of labeling pattern.

[andrewhayward]

I'd need to spend more time looking at how BaseControl works to offer much feedback around any of this!

[andrewhayward]

I'm going to recap the changes in this PR, mainly to confirm with you whether I'm understanding correctly what you're proposing, and to help other reviewers:
[...]

This all looks correct, yes.

Typically, I've seen the aria-controls attribute being used for composite widgets like comboboxes — this approach is definitely interesting, and one that we haven't explored yet! I'd be very curious to test it further across different browsers / OSs / screen readers. Have you already done some testing?

I've done some limited testing, yes. It is my understanding though that only JAWS (don't have access currently) has an option for jumping to controlled elements, and even then doesn't announce the presence of any such relationships by default.

The most interesting and challenging aspect of this approach is in evaluating its scalability / composability. A few considerations:

• UnitControl itself renders multiple input elements, in a "composite" way (ie. the quantity and the unit inputs)
• RangeControl can render a redundant input field itself (when the withInputField prop is set to true)

Would this approach still work well when wrapping around (or nesting under) other similar composite and/or redundant components?

It should do, yes (caveat: my awareness of existing components is somewhat cursory!), as long as IDs and aria-* props are appropriately applied to the correct elements within components.

[alexstine]

@andrewhayward Sorry for taking so long to review this. Could you please refresh the PR?

Thanks.

[github-actions]

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

Unlinked Accounts

The following contributors have not linked their GitHub and WordPress.org accounts: @andrew-palapa.

Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Unlinked contributors: andrew-palapa.

Co-authored-by: alexstine <alexstine@git.wordpress.org>
Co-authored-by: ciampo <mciampini@git.wordpress.org>
Co-authored-by: mirka <0mirka00@git.wordpress.org>
Co-authored-by: andrewhayward <andrewhayward@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

[alexstine]

@andrewhayward I think this one looks good, can we get it merged? Gave it a test in storybook, all looks good.

[andrewhayward]

I think this one looks good, can we get it merged? Gave it a test in storybook, all looks good.

I totally forgot about this PR! Thanks for chasing, I'll get it sorted out.

[ciampo]

Closing in favour of #63761, a copy of this PR associated to a branch on the Gutenberg repository for easier collaboration from all contributors. I'll make sure to credit this PR's author and collaborators in case the clone PR gets merged.