Adds FormControl's "Auto-Wiring" to SelectPanel v2 Component

[JeffreyKozik]

Followup to this conversation in Slack and #4372. Related to https://github.com/github/primer/issues/2396.

Changelog

New

Changed

Made use of the the new useFormControlForwardedProps hook (introduced by #3632) to automatically wire the SelectPanel v2 component to the accessibility and validation provided by the FormControl component it's nested within. If SelectPanel v2 isn't nested within FormControl the hook has no effect.

Within a form the accessible name is now the text within the corresponding FormControl.Label tag and the button itself

Outside of a form the accessible name is still the text within the SelectPanel.Button itself

Removed

Rollout strategy

• Patch release
• Minor release
• Major release; if selected, include a written rollout or migration plan
• None; if selected, include a brief description as to why

Testing & Reviewing

Merge checklist

• Added/updated tests
• Added/updated documentation
• Added/updated previews (Storybook)
• Changes are SSR compatible
• Tested in Chrome
• Tested in Firefox
• Tested in Safari
• Tested in Edge
• (GitHub staff only) Integration tests pass at github/github (Learn more about how to run integration tests)

[changeset-bot]

🦋 Changeset detected

Latest commit: eacfa7c

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@primer/react	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[JeffreyKozik]

Followed the pattern suggested in this slack thread: https://github.slack.com/archives/CSGAVNZ19/p1709843761424779?thread_ts=1709836721.563139&cid=CSGAVNZ19

[JeffreyKozik]

Using the hook recently created by @iansan5653 in this PR: #3632. This hook is also being used in the InlineAutocomplete draft component:

react/packages/react/src/drafts/InlineAutocomplete/InlineAutocomplete.tsx

Line 118 in cb54f42

const inputProps = useFormControlForwardedProps(externalInputProps)

[github-actions]

size-limit report 📦

Path	Size
packages/react/dist/browser.esm.js	88 KB (0%)
packages/react/dist/browser.umd.js	88.25 KB (0%)

[siddharthkp]

Looks good!

Merging to main is blocked today for another release. I can merge this tomorrow.

[JeffreyKozik]

Looks good!

Merging to main is blocked today for another release. I can merge this tomorrow.

Sounds great, thank you!

[adrianababakanian]

🎉

[iansan5653]

🤔 Overall the approach looks great, but I have concerns about the accessibility implications. If a button's label is set using aria-label or aria-labelledby, the text inside the button is totally overridden and becomes invisible to screen readers.

This is problematic for several reasons:

• There may be additional context / instructions in the panel's placeholder value that's shown when no selection is active, and designers may rely on users being able to access this content
• In setups where the current selection is used as the anchor text, when a selection is made, screen reader users will be unable to determine what it is (or even that anything is selected)
• Voice control users will see a button and think that its content is its label, but actually the label is something different (this may be more of an inherent problem with the way we style SelectPanel anchors as buttons instead of inputs)

---

A potential alternative: What if instead of setting aria-labelledby, we set the button's aria-label to something like "Label Text: Button Text"? This would be a bit of a hack; we'd have to do something roughly like `${document.getElementById(labelledBy).textContent}: ${buttonText}`. This would result in the following label states in your Storybook example:

• Without selection: "SelectPanel within FormControl: Choose a tag"
• With selection: "SelectPanel within FormControl: v35.28.0"

However there are risks with this approach too: will users be confused by the accessible label of the element changing when the selected value changes? I don't know how this would affect VoiceOver users.

It's also possible this could create confusing labels, like if the consumer sets the visible label and the button text to the same value: "Choose a tag: Choose a tag" would be annoying. IMHO, the potential for annoyance is probably outweighed by the potential for totally excluding important context.

---

A third option might be to explore a different role altogether for the anchor of a SelectPanel. I'm not totally sure that button is actually ideal here when the current selection is used as the anchor text.

On one hand yes, it's an activatable button that opens a panel; but on the other hand it's also an input with a visible current value. Is there a role we can use that would allow us to set both a label and a current value?

I think it's worth digging in to this a bit further - maybe in accessibility office hours?

[JeffreyKozik]

@iansan5653 thank you for the detailed and thoughtful review.

As we use the FormControl.Label to label other components within forms such as radio groups, text inputs, and select menus, why is the labelling able to work in an accessible way for those components, but not for this one? Does it ultimately come down to those being role=input and this being role=button?

If that's the case would it either a) be appropriate to change to role=input for the entire SelectMenu or b) be appropriate to have a prop that allows developers to set to role=input for situations such as this one?

Alternatively, the 2nd option you propose seems acceptable as well.

I'm happy to go to a11y office hours to discuss this further if needed as well. Let me know the next one you're available for.

[iansan5653]

Does it ultimately come down to those being role=input and this being role=button?

Yep, pretty much. While both roles are widgets with accessible names and text content, screen readers intentionally ignore the text content of a button in favor of its accessible name. 99% of the time these two things are identical (because it's not really standard to have a button with an external label), so if a screen reader were to read out both it would be annoying and unnecessary the vast majority of the time .

If that's the case would it either a) be appropriate to change to role=input for the entire SelectMenu or b) be appropriate to have a prop that allows developers to set to role=input for situations such as this one?

Unfortunately not. role=input implies much more than just 'this button acts similarly to an input'. Screen readers will expect an input role to act exactly like a text input - that is, they will expect it to accept text content and not provide a list of options.

Native browser <select> elements get the role combobox, which is much a closer candidate. I think the only thing SelectPanel is missing from the combobox pattern is handling alphanumeric keys when focus is on the anchor when the [panel is closed (this should probably open the panel and put those characters in the filter). Here's an example of an accessible select-like custom combobox.

It's honestly possible that we should consider always giving the SelectPanel anchor the role of combobox, or maybe automatically in certain situations. I really don't know for sure about that and I'm by no means an expert, so I'd definitely recommend talking to the accessibility team about that. I'd be happy to show up to any of their office hours - just let me know when you'll be there!

[JeffreyKozik]

@iansan5653 that's interesting context. It seems like giving SelectPanel the combobox role makes a lot of sense here, although I'm not sure the implications of that for the other use cases of SelectPanel such as when it's outside of a form. @siddharthkp what do you think about giving the SelectPanel anchor the role of combobox?

[TylerJDev]

I believe it was mentioned that SelectPanel couldn't be a combobox, even though it's a similar pattern, as it handles certain aspects differently than a true combobox would.

We do have something else in the works that could act as the middle ground between SelectPanel and ActionMenu, that's specifically meant to be a true "combobox". Since this is still in a proof of concept state, I think it'd be best to try a different route for now, and iterate on it later.

I'm wondering if we could keep the usage of aria-labelledby, but also reference the button itself. For example:

<label id="label1">SelectPanel within FormControl</label>
<button id="label2" aria-labelledby="label1 label2">Choose a tag</button>

This means that the button references both the label and the contents within itself (i.e. "Choose a tag"), making the accessible name: "SelectPanel within FormControl, Choose a tag". This is using multiple aria-labelledby id(s) to concatenate a label.

We could also change the order so that the current value is announced first (e.g. aria-labelledby="label2 label1" = "Choose a tag, SelectPanel within FormControl"). If we went this route, this would have to be unique to SelectPanel, as other the other components that support FormControl linking now do not require additional ARIA.

I'd still recommend running this by the direct Accessibility team, as I may be unaware of any other implications, such as how someone using voice might interact with this control, and Ian's comments around potential confusion with the label. I don't mind following up with them too!

[siddharthkp]

This is great @iansan5653, thanks for bringing this up! I totally missed it.

Without selection: "SelectPanel within FormControl: Choose a tag"
With selection: "SelectPanel within FormControl: v35.28.0"

This looks perfect! There is a really old discussion that might be helpful to read through: https://github.com/github/primer/discussions/696

I'm wondering if we could keep the usage of aria-labelledby, but also reference the button itself. For example:

SelectPanel within FormControl
Choose a tag

This would work nicely!

@TylerJDev, Just checking, both of these information should be considered as labels right (not description)?

It's honestly possible that we should consider always giving the SelectPanel anchor the role of combobox

As Tyler said, it's something still being discussed with our accessibility consultants, we may or may not end up there. It's tricky because SelectPanel also has other elements like filter buttons (that look like tabs).

Let's park that for a couple weeks till we can conclude if we're going the combobox route or not.

@JeffreyKozik how do you feel about making this change? If you've got other things on your plate, I can help out here!

[JeffreyKozik]

@TylerJDev @siddharthkp thanks for providing more details on this issue! I'll take a swing at this change later today, but if I get pulled into other issues and need you to help out here on Monday I'll let you know, thanks @siddharthkp!

[TylerJDev]

Just checking, both of these information should be considered as labels right (not description)?

Yup, exactly! If we used a description for one or the other, there's the possibility that it would be ignored by a screen reader (or other AT).

[JeffreyKozik]

@siddharthkp if this approach looks suitable to you, I think it's ready to merge 👍

[JeffreyKozik]

Trying to create the integration tests and run them again with my updated code, however the workflow keeps failing: https://github.com/github/github/actions/workflows/primer-react-pr-test.yml so I'm not even able to run the integration tests. Not sure if this is related to what's ongoing with npm, or a separate issue

It's working as expected now

[JeffreyKozik]

Integration tests passed 🎉 https://github.com/github/github/pull/322332

Only tests failing seem related to github-licenses which I believe is expected because it's an integration test PR