Skip to content

Conversation

@TylerJDev
Copy link
Member

Closes https://github.com/github/primer/issues/5083

Adds role="option" if role="listbox" is applied to ActionList. This stemmed from some instances of ActionList utilizing role="listbox" without adding role="option". This broke functionality, as usage of selectionVariant requires either aria-checked being present, or role="option".

Since we can infer what child roles should be based on the parent role, this PR adds role="option" if role="listbox" is present

Changelog

New

  • Adds role="option" if role="listbox" is present in ActionList

Changed

  • Allows ActionList.Item(s) to be focused even if inferredItemRole is present

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

@changeset-bot
Copy link

changeset-bot bot commented May 15, 2025

🦋 Changeset detected

Latest commit: 7e4a18e

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 github-actions bot added staff Author is a staff member integration-tests: recommended This change needs to be tested for breaking changes. See https://arc.net/l/quote/tdmpakpm labels May 15, 2025
@github-actions
Copy link
Contributor

👋 Hi, this pull request contains changes to the source code that github/github depends on. If you are GitHub staff, we recommend testing these changes with github/github using the integration workflow. Thanks!

@github-actions
Copy link
Contributor

github-actions bot commented May 15, 2025

size-limit report 📦

Path Size
packages/react/dist/browser.esm.js 96.19 KB (+0.05% 🔺)
packages/react/dist/browser.umd.js 96.51 KB (+0.03% 🔺)

@primer-integration
Copy link

👋 Hi from github/github! Your integration PR is ready: https://github.com/github/github/pull/378331

@primer-integration
Copy link

🔴 golden-jobs completed with status failure.

@github-actions github-actions bot added integration-tests: failing Changes in this PR cause breaking changes in gh/gh and removed integration-tests: recommended This change needs to be tested for breaking changes. See https://arc.net/l/quote/tdmpakpm labels May 15, 2025
@Rulasz0705

This comment was marked as off-topic.

@TylerJDev TylerJDev marked this pull request as ready for review May 16, 2025 15:32
Copilot AI review requested due to automatic review settings May 16, 2025 15:32
@TylerJDev TylerJDev requested a review from a team as a code owner May 16, 2025 15:32
@TylerJDev TylerJDev requested a review from joshblack May 16, 2025 15:32
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Adds automatic role="option" for ActionList.Item when its parent ActionList is a listbox, and adjusts focus logic and tests accordingly.

  • Extends inferred item role logic to detect any listRole === 'listbox'.
  • Simplifies disabled/inactive focusable branch.
  • Updates tests and stories to validate and demonstrate the new behavior.

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
packages/react/src/ActionList/Item.tsx Simplify and extend role inference logic for listbox contexts
packages/react/src/ActionList/Item.test.tsx Remove hardcoded role="option" and add assertion for inferred option roles
packages/react/src/ActionList/ActionList.test.tsx Update disabled item focus expectation in keyboard navigation tests
packages/react/src/ActionList/ActionList.features.stories.tsx Rename story, show explicit role="option" (to illustrate) under listbox
.changeset/soft-webs-study.md Update changelog with new inference and focusable behavior
Comments suppressed due to low confidence (2)

packages/react/src/ActionList/ActionList.features.stories.tsx:332

  • [nitpick] The story name uses inconsistent casing for 'Listbox'. Consider renaming to ListBoxSingleSelect to match the established PascalCase convention (ListBoxMultiSelect).
export const ListboxSingleSelect = () => {

packages/react/src/ActionList/Item.test.tsx:425

  • Consider adding a corresponding test for the multiple selectionVariant="multiple" case to ensure role="option" is inferred correctly for multi-select listboxes.
it('should add `role="option"` if `role="listbox"` and `selectionVariant` is present', async () => {

else inferredItemRole = 'menuitem'
} else if (container === 'SelectPanel' && listRole === 'listbox') {
if (selectionVariant !== undefined) inferredItemRole = 'option'
} else if ((container === 'SelectPanel' && listRole === 'listbox') || listRole === 'listbox') {
Copy link

Copilot AI May 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This conditional can be simplified to if (listRole === 'listbox'), since the extra SelectPanel check is redundant.

Suggested change
} else if ((container === 'SelectPanel' && listRole === 'listbox') || listRole === 'listbox') {
} else if (listRole === 'listbox') {

Copilot uses AI. Check for mistakes.
aria-checked={selectedIndice === index}
onSelect={() => handleSelect(index)}
disabled={index === 3 ? true : undefined}
role="option"
Copy link

Copilot AI May 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] Since role="option" is now inferred when ActionList has role="listbox", this explicit prop in the story is redundant and could be removed for clarity.

Suggested change
role="option"

Copilot uses AI. Check for mistakes.
@TylerJDev TylerJDev added this pull request to the merge queue May 19, 2025
Merged via the queue into main with commit 776e05e May 19, 2025
35 checks passed
@TylerJDev TylerJDev deleted the tylerjdev/add-implicit-role-for-listbox-action-list branch May 19, 2025 19:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

integration-tests: failing Changes in this PR cause breaking changes in gh/gh staff Author is a staff member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants