feat(a11y): give RadioGroup aria "radiogroup" role

[bvandercar-vt]

Fixes #0000

Checklist

• [N/A] Includes tests
• [N/A] Update documentation

Changes proposed in this pull request:

RadioGroup component should be getting role="radiogroup". Currently, it's just a div. Everything else about it works in terms of a radiogroup, ie keypresses, attributes, etc.

[ggdouglas]

I'm generally supportive of the direction of this PR since it improves accessibility, but I also think we should more carefully consider how we approach the composition of the label component in the radio group.

I also support adding role="radiogroup" to the component since it should generally be there. Could you split that part of this PR out so that we can expedite getting that fixed?

[ggdouglas]

Could you explain the reasoning around using Pick<React.HTMLAttributes<Element> and React.AriaAttributes for this prop interface?

Could we alternately achieve this using something like HTMLDivProps?

blueprint/packages/core/src/common/props.ts

Line 29 in 4111fc8

export type HTMLDivProps = React.HTMLAttributes<HTMLDivElement>;

[bvandercar-vt]

Could we alternately achieve this using something like HTMLDivProps?

Absolutely! Changed.

[ggdouglas]

I think we're missing a few other props here not included in removeNonHTMLProps. Namely, selectedValue and disabled are included on this div where they shouldn't.

Can you include "selectedValue" in the INVALID_PROPS array of removeNonHTMLProps and also add disabled to the destructuring statement above so that it is omitted from the rendered div?

blueprint/packages/core/src/common/props.ts

Lines 130 to 163 in 4111fc8

	const INVALID_PROPS = [
	"active",
	"alignText",
	"asyncControl", // InputGroupProps
	"containerRef",
	"current",
	"elementRef", // not used anymore in Blueprint v5.x, but kept for backcompat if consumers use this naming pattern
	"ellipsizeText", // ButtonProps
	"fill",
	"icon",
	"iconSize",
	"inputClassName",
	"inputRef",
	"intent",
	"inline",
	"large",
	"loading",
	"leftElement",
	"leftIcon",
	"minimal",
	"onRemove", // TagProps, TagInputProps
	"outlined", // ButtonProps
	"panel", // TabProps
	"panelClassName", // TabProps
	"popoverProps",
	"rightElement",
	"rightIcon",
	"round",
	"size",
	"small",
	"tagName",
	"text",
	"textClassName", // ButtonProps
	];

[bvandercar-vt]

Done

[ggdouglas]

Is it problematic that we now can pass both aria-labelledby and label to this component? Take the following example:

<label id="external-label">My Label</label>
<RadioGroup
    label={<label id="prop-node-label">Determine lunch</label>}
    aria-labelledby="external-label"
    ...
>
    <Radio label="Soup" value="one" />
    <Radio label="Salad" value="two" />
    <Radio label="Sandwich" value="three" />
</RadioGroup>

Which "label" should take precedence?

Furthermore, we can theoretically achieve the proper aria labeling by passing in a label manually.

<RadioGroup
    aria-labelledby="my-label"
    ...
>
    <Label id="my-label">My Label</Label>
    <Radio label="Soup" value="one" />
    <Radio label="Salad" value="two" />
    <Radio label="Sandwich" value="three" />
</RadioGroup>

IMO, the functionality of the label prop itself seems a bit ambiguous for this case. Perhaps it would ultimately be better if <RadioGroup> did away with this prop and instead associated any <Label> components passed as children with the radio group.

[bvandercar-vt]

Is it problematic that we now can pass both aria-labelledby and label to this component?

No, this allows a consumer to pass a custom aria-labelledby if they wish to use an external label.

Which "label" should take precedence?

In your example, external-label takes precedence because the consumer intentionally passed the aria-labelledby prop. which is exactly the way they should do it if they want an external label. The internal label no longer labels it, but that's exactly what they wanted when they passed aria-labelledby themselves.

Furthermore, we can theoretically achieve the proper aria labeling by passing in a label manually.

Yes your second example is exactly what is happening. Via uniqueId. The code you wrote here is the code that it is.

IMO, the functionality of the label prop itself seems a bit ambiguous for this case.

I disagree, it's pretty clear as to what it does. It creates a label for the radiogroup.

and instead associated any components passed as children with the radio group.

This would involved refactoring the entire component. If you do want to make that change it would be a major version change. But there really is no scenario where you'd want to include the label in a location other than where it already is, as the first child.

[bvandercar-vt]

@ggdouglas looks like this PR has been forgotten about, can you take a look?