The HeightControl is unlabeled

[afercia]

Description

Yet another case of unlabeled controls. Cc @ciampo

Discovered while working on #57680

The HeightControl uses two components under the hood:

• UnitControl.
• RangeControl

They are both totally unlabeled, as HeightControl dosn't pass any label to them
The components themselves are open to misues, as omitting the label prop doesn't throw any error / warning / deprecation.

Unlabeled controls are not acceptable after almost 7 years of life of this project. This is basic accessibility and I would say basic HTML.

Step-by-step reproduction instructions

• Select a block that provides a height control e.g. a Group block.
• Observe the Min. Height control in the Settings Panel > Styles tab > Dimensions.
• Inspect the source in the dev tools.
• Observe that both the input and the range controls are unlabeled:
  • There is no associated label element.
  • There is no aria-label element.
  • There is no aria-labelledby attribute.
• One of the three labeling mechanisms above must be provided for any interactive control.

Screenshots, screen recording, code snippet

Current state on trunk, notice the lack of a label prop or aria-label attribute:

UnitControl in the HeightControl:

gutenberg/packages/block-editor/src/components/height-control/index.js

Lines 153 to 160 in 169e2e5

	<UnitControl
	value={ value }
	units={ units }
	onChange={ onChange }
	onUnitChange={ handleUnitChange }
	min={ 0 }
	size={ '__unstable-large' }
	/>

RangeControl in the HeightControl:

https://github.com/WordPress/gutenberg/blob/trunk/packages/block-editor/src/components/height-control/index.js/#L164-L178

Environment info

No response

Please confirm that you have searched existing issues in the repo.

Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

Yes

[afercia]

While the fix is trivial, this bug proves yet one more time that:

• Contributors and reviewers in this project are not trained / educated / do not have enough knowledge to understand and provide appropriate labeling for interactive controls.
• The base components are open to misuse as it's possible to entirely omit the label prop thus rendering unlabeled controls.

Besides making Gutenberg not accessible, I would like to remind the editor packages are public and used also outside of the Gutenberg project (at least potentially). As such, releasing components that are open to misuse doesn't contribute to make the Web a better place. Rather, it makes it worse.

Cc @joedolson @alexstine @andrewhayward

[alexstine]

CC: @annezazu . I think we're reaching a point with this where it needs to have a wider discussion with leadership. I don't wish to offend anyone in the project but Andrea is right. These components can be used outside of the editor and there should be no way to exclude a label for accessibility. It could be a hidden aria-label but some type of label.

[afercia]

I'd like to add that it's pretty saddening and not motivating at all that this bug is in the codebase since more than one year and no one noticed it so far.
There's no automated tests in place, no accessibility checks (aXe) in place other than in the Storybook and evidently not manual testing during implementation and review.

Once again, this is not complex. It's basic HTML. And we're failing at it.

Honestly, I think we're at a point where either this project needs to make sure this kind of bugs will not happen again or, not my intent to offend anyone, this project needs to consider to restrict merge permission only to contributors who can assure some higher code quality.

[ciampo]

Hey folks, thank you for your continued effort in flagging all these accessibility violations.

As there already is an open issue (#51740) where the general topic is being actively discussed, we should probably continue the conversation there to avoid repetition and keep the focus.

I would also kindly ask you not to be pinged on every single labelling-related issue, as I'm definitely not in charge of single-handedly maintaining this part of the codebase (and we already have a separate issue).

[ramonjd]

The fix seemed pretty straight forward. I've thrown up a PR here:

• Design tools: add aria labels to the inputs of Height Control #57695

Apologies, I didn't see the PR linked above 🤦🏻

I've closed my PR

[andrewserong]

Thanks for the discussion here, folks, I think this one might have been my oversight when originally introducing the component to pair a UnitControl with a RangeControl for the minimum height controls. Previous to #45875 we were using a single UnitControl for the minimum height controls with a label passed to it, and then in #45875 the label was moved "up" to a legend within a wrapping fieldset for these two controls. However that change failed to pass a label down to the individual UnitControl and RangeControl components within it.

Good to know that it was missed and that in that scenario we still need to have labels on each individual controls within that fieldset, even if the labels aren't visible.

In this case, what is the desired label for each component? We have a legend at the wrapper level (e.g. Minimum Height). Would we re-use that same label for the UnitControl and RangeControl (as they're both changing the same value), or would a more specific label for each of those controls be preferred?

[andrewserong]

Edit: oh, I see there's a fix open already in: #57683 that passes down the label. I'll give that a review. Thanks @afercia!

I'll be sure to keep an eye out for similar issues in other refactors and reviews.

[noisysocks]

• Contributors and reviewers in this project are not trained / educated / do not have enough knowledge to understand and provide appropriate labeling for interactive controls.

An accessibility bug doesn't mean that contributors lack accessibility training. That would be like saying a logic error means that contributors can't program or a typo means that contributors can't spell. It's a mistake, they're inevitable, please be kind and assume positive intent.

[alexstine]

@noisysocks I agree sometimes delivery isn't great and it should have been phrased better but it's also like saying let's remove PHPUnit and lint checks from Core and good luck. As long as Gutenberg has been around, there should be more solid tests around accessibility and we should be designing in such a way that devs get big nasty warnings when something might be inaccessible so that way mistakes are less likely to happen. I agree, I don't think the blame helps much here but it would be nice to start seeing some things evolve in the right direction. We're all people, we're all going to make mistakes. Let's figure out how to implement technology/automation in such a way that mistakes are less likely to happen.

If you look back in some previous PRs, even I have made things worse from time to time or broken Voiceover when it worked on Windows.

Thanks.

[noisysocks]

Yep absolutely. Automated testing for common a11y errors sounds like it would be valuable. Is it something you're interested in exploring?

[alexstine]

@noisysocks It has been attempted before but never really made it completely. I don't have a lot of JavaScript experience as front-end development isn't my day job so I wouldn't really know how to drive a goal like that.