Font Library: Fix checkboxes labeling in the Fonts modal dialog

[afercia]

Description

Noticed while reviewing #58212

To recap some HTML basics:
The HTML <label> element can be associated to a form control in two ways:

• Either by using a for attribute that references the id attribute of the form control. This is sometimes called 'explicit labeling'.
• Or by wrapping the form control within the <label> element. This is sometimes called 'imnplicit labeling'.

It's a either / or. Never do both things.

The WordPress accessibility best practices recommend explicit labeling with for / id attributes. See https://make.wordpress.org/accessibility/handbook/markup/core-accessibility-standards/#labeling

In the Fonts modal the checkboxes for the fonts use both explicit and implicit label, that is: the <label> element wraps the checkbox and also used for / id attributes. I do realie the intent was to make the entire font row clickable but this needs to be changed.

Also, the editor components come with built-in handling of labeling mechanism. I'm not sure altering the default behavior of the components in such a way is in line with the whole purpose of using reusable components. This kind of override of the default behavior should not be allowed. Moreover, setting the label prop to false label={ false } seems to me something that should not be allowed. I could find only two occurrences of label={ false } in the codebase, both in the fonts modal dialog, plus a related test.

Cc @ciampo I'm not sure I understand why a false value is allowed, it doesn't seem to me the best way to make sure inputs are properly labeled. This issue proves that if we make the component accept a false value, there are chances that developers will end up using the component in incorrect ways. Not to mention the BaseControl and InputControl label prop is typed to ReactNode and instead accepts also false?

Related code to fix:

gutenberg/packages/edit-site/src/components/global-styles/font-library-modal/collection-font-variant.js

Lines 38 to 52 in ddde310

	<label
	className="font-library-modal__library-font-variant"
	htmlFor={ checkboxId }
	>
	<Flex justify="flex-start" align="center" gap="1rem">
	<CheckboxControl
	checked={ selected }
	onChange={ handleToggleActivation }
	__nextHasNoMarginBottom={ true }
	id={ checkboxId }
	label={ false }
	/>
	<FontFaceDemo fontFace={ face } text={ displayName } />
	</Flex>
	</label>

and

gutenberg/packages/edit-site/src/components/global-styles/font-library-modal/library-font-variant.js

Lines 48 to 62 in ddde310

	<label
	className="font-library-modal__library-font-variant"
	htmlFor={ checkboxId }
	>
	<Flex justify="flex-start" align="center" gap="1rem">
	<CheckboxControl
	checked={ isInstalled }
	onChange={ handleToggleActivation }
	__nextHasNoMarginBottom={ true }
	id={ checkboxId }
	label={ false }
	/>
	<FontFaceDemo fontFace={ face } text={ displayName } />
	</Flex>
	</label>

Step-by-step reproduction instructions

• See the code linked above or:
• Go to Site editor > Styles > Typography > Manage fonts
• The fonts modal dialog opens
• Click a font to see the font details sub-view
• Inspect the checkboxes in your browser dev tool sinspector.
• Observe the label element of the checkboxes wraps the checkbox inpug and has a for attribute pointing to the id of the input.

Screenshots, screen recording, code snippet

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]

Introduced in #56158 where I see the documentation of the CheckboxCobtrol for the lebel prop was changed from string to string|false. See

https://github.com/WordPress/gutenberg/pull/56158/files#diff-75e8332625ebfa6eacc21992b3dc73f6b7b7c99bac1980bf0f525da9dee29811R80

The entry in the changelog mentions: 'Add option to not render label' which is arguable a good idea for accessibility 😞

[ciampo]

Cc @ciampo I'm not sure I understand why a false value is allowed, it doesn't seem to me the best way to make sure inputs are properly labeled.

As you also found out, the change was not carried out nor reviewed by @WordPress/gutenberg-components , and the PR itself seems quite self-explanatory about the author's intentions.

This issue proves that if we make the component accept a false value, there are chances that developers will end up using the component in incorrect ways.

The PR author's intentions come from a good place — providing an alternative label to the component. I agree that we could have discussed the issue and thought about a solution together, before merging that PR.

Not to mention the BaseControl and InputControl label prop is typed to ReactNode and instead accepts also false?

That shouldn't be an issue since ReactNodes can also be a boolean:

https://github.com/DefinitelyTyped/DefinitelyTyped/blob/7fdfd25b1d74ab4091127c3bc854c1ce017de55c/types/react/index.d.ts#L245-L256

[afercia]

That shouldn't be an issue since ReactNodes can also be a boolean:

Thanks for clarifying I'm not very familiar with TypeScript and it shows.

As i noted on #56158 (comment) i do understand the goo dintent. However, I'm not sure not following this project guidelines should be allowed. Speficially: that PR doesn't have an associated issue to allow for borader discussion. Also, it's not marked with the Accessibility labels.