More menu "feature toggle" invalid HTML

[afercia]

Noticed while reviewing #13059

The FeatureToggle component renders a label HTML attribute, which is invalid HTML. Seems to me the label prop should only be used as children and not passed as a prop to MenuItem:

gutenberg/packages/edit-post/src/components/header/feature-toggle/index.js

Lines 23 to 33 in 0f18024

	return (
	<MenuItem
	icon={ isActive && 'yes' }
	isSelected={ isActive }
	onClick={ flow( onToggle, speakMessage ) }
	role="menuitemcheckbox"
	label={ label }
	info={ info }
	>
	{ label }
	</MenuItem>

otherwise, it gets rendered as an HTML attribute (which doesn't do anything other than invalidating the HTML):

[draganescu]

hey @afercia so the change to the FeatureToggle component should only be removing it as a prop?

return ( 
 	<MenuItem 
 		icon={ isActive && 'yes' } 
 		isSelected={ isActive } 
 		onClick={ flow( onToggle, speakMessage ) } 
 		role="menuitemcheckbox" 
 		info={ info } 
 	> 
 		{ label } 
 	</MenuItem>

and that's that?

Apparently the menu item component accepts a label to:

### `label`

- Type: `string`
- Required: No

String to use as primary button label text, applied as `aria-label`. Useful in cases where an `info` prop is passed, where `label` should be the minimal text of the button, described in further detail by `info`.

Defaults to the value of `children`, if `children` is passed as a string.

From your screenshot it appears not to end up as aria-label but simply label.

[afercia]

Hey @draganescu thanks for looking into this. Checking code history, the MenuItem label prop has been removed in #12955, see the related issue #12501.

Looks like there's some more work to do, e.g. the documentation hasn't been updated 🙂

The MenuItem should be cleaned up a bit now, as the previous argumentation "Useful in cases where an info prop is passed, where label should be the minimal text of the button, described in further detail by info." doesn't apply any longer.

This was used for some settings, for example in the more menu:

where the item used to have:

• aria-label="Top Toolbar"
• aria-describedby pointing to the info text

The intent was to make screen reader announce the item in the following way:

Top Toolbar, menu item 
{brief pause}
Access all block and document tools in a single place

However, that's a bit overkill. Screen reader users usually set their screen reader voice rate to a high value and the brief pause is a bit pointless. There's basically no great difference in letting screen readers announce the whole text in one go, e.g.:

Top Toolbar Access all block and document tools in a single place, menu item 

I'd rather simplify and:

• remove from MenuItem the id, infoId, and aria-describedby things
• update the documentation 🙂

[Jackie6]

@afercia Hi! Thanks for summarizing the changes on MenuItem. I have created a pull request to fix this issue.