Add aria-selected value to ActionList.Item.

[radglob]

By adding aria-selected to the <li role="option"> tags, NVDA and other screen readers will now announce that an option is "not selected" when aria-selected=false.

From what I've read, it is not the normal function of NVDA or JAWS to announce that an option is selected. Silence is the default behavior. https://stackoverflow.com/questions/71468332/select2-li-aria-selected-true-is-not-read-as-selected-by-screen-reader

Without aria-selected, state will not be announced for any option.

Closes https://github.com/github/accessibility-audits/issues/4484.

Screenshots

Please provide before/after screenshots for any visual changes

Merge checklist

• Added/updated tests
• Added/updated documentation
• Changes are SSR compatible
• Tested in Chrome
• Tested in Firefox
• Tested in Safari
• Tested in Edge

Take a look at the What we look for in reviews section of the contributing guidelines for more information on how we review PRs.

[changeset-bot]

🦋 Changeset detected

Latest commit: 25ef26b

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	Patch

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

[github-actions]

size-limit report 📦

Path	Size
dist/browser.esm.js	103.84 KB (+0.03% 🔺)
dist/browser.umd.js	104.38 KB (+0.02% 🔺)

[broccolinisoup]

FYI: I was just running tests with the new primer action and used this PR to test its integration with github/github and all checks are passing 🤩 https://github.com/github/github/pull/283677

[owenniblock]

I noticed while looking at this issue during a pairing session that the example: https://primer-9eeb77613b-13348165.drafts.github.io/storybook/?path=/story/components-actionlist-examples--async-list-with-spinner isn't adding the selected attribute to the option element (the code is there but for some reason the HTML output doesn't include selected. We didn't investigate whether this was a code error in the example or an issue with the component but it might be worth checking why this is happening while we're looking at this aspect of the code.

[radglob]

@owenniblock I don't think the selected attribute is needed here since this isn't a select element. As I understand, the aria-selected attribute is used with listbox, but selected itself won't actually do anything.

[owenniblock]

@radglob you are right! Sorry for the confusion. I'm slightly confused by the HTML specs not saying that options are valid inside a listbox but only valid inside select, datalist or optgroup elements and yet the WAI ARIA listbox examples behave as we plan to do stuff here. I'm going to do a bit more research here to try and work out why there's a discrepancy between these two documents.

EDIT: https://w3c.github.io/aria/#listbox states:

Items within the list are static and, unlike standard HTML select elements, can contain images. List boxes contain children whose role is option or elements whose role is group which in turn contain children whose role is option.

Aha! This is OK because:

Superclass Role: select

So listbox is basically a select with things on top.

We should consider removing the selected code from the examples if we're not expecting those to work.

[owenniblock]

Link to code line I was referencing that tries to add selected

[radglob]

@owenniblock I think we still need that. In this case, selected is not intended to be the HTML attribute. It's just a prop here that we use to communicate whether or not an item is selected. We then use that value to populate the aria-selected attribute in the correct place, render the ActionList.Selection component properly, etc.

[owenniblock]

@radglob I was just about to post to say sorry 😂 I totally misread what that code was going (I just went over the code in Item.tsx). Looks good to me, I even worked out why the aria and HTML specs were confusing me.

[joshblack]

Looking great! Just left a small comment for clarity, also wanted to ask if you could add a test case for this scenario? Would help a ton to make sure we don't regress in the future 🙏