Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes for ActionList semantics #4272

Merged
merged 48 commits into from
Jun 18, 2024
Merged

Conversation

TylerJDev
Copy link
Contributor

@TylerJDev TylerJDev commented Feb 14, 2024

Fixes issue in: https://github.com/github/accessibility-audits/issues/2939

Based off of @broccolinisoup's work in #3971

Ensures that the default semantics of ActionList.Item rely on <button> instead of <li tabindex="0">. If the ActionList itself should act as a list or menu, <li> will be used with the appropriate roles.

Changed

  • Default element type of ActionList.Item is now button
  • Minor type changes (i.e. HTMLLIElement to HTMLButtonElement)
  • Small ARIA addition to NavList, AutocompleteSuggestions.

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

Integration test PR: https://github.com/github/github/pull/317139

Merge checklist

Copy link

changeset-bot bot commented Feb 14, 2024

🦋 Changeset detected

Latest commit: 0298349

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

@github-actions github-actions bot temporarily deployed to storybook-preview-4272 February 15, 2024 15:30 Inactive
@TylerJDev TylerJDev added the skip changeset This change does not need a changelog label Feb 15, 2024
Copy link
Contributor

github-actions bot commented Feb 15, 2024

size-limit report 📦

Path Size
packages/react/dist/browser.esm.js 89.89 KB (+0.3% 🔺)
packages/react/dist/browser.umd.js 90.2 KB (+0.31% 🔺)

dependabot bot and others added 6 commits February 20, 2024 16:30
Bumps [changesets/action](https://github.com/changesets/action) from 1.4.5 to 1.4.6.
- [Release notes](https://github.com/changesets/action/releases)
- [Changelog](https://github.com/changesets/action/blob/main/CHANGELOG.md)
- [Commits](changesets/action@f13b1ba...e2f8e96)

---
updated-dependencies:
- dependency-name: changesets/action
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
* fix(SelectPanel2): add aria-labelledby to listbox

* test(e2e): add e2e test for SelectPanel2 default story

* chore: add changeset

* test(vrt): update snapshots

---------

Co-authored-by: Josh Black <[email protected]>
* small bug fixes with v8

* Create khaki-schools-lay.md

* test(vrt): update snapshots

* snippy snaps

* test(vrt): update snapshots

* test: update snapshots

* test: update snapshots

* try commenting flakey tests

* test: comment out flakey snapshot test

---------

Co-authored-by: langermank <[email protected]>
Co-authored-by: Josh Black <[email protected]>
Bumps [ip](https://github.com/indutny/node-ip) from 2.0.0 to 2.0.1.
- [Commits](indutny/node-ip@v2.0.0...v2.0.1)

---
updated-dependencies:
- dependency-name: ip
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
@@ -250,8 +256,22 @@ export const Item = React.forwardRef<HTMLLIElement, ActionListItemProps>(
const inlineDescriptionId = `${itemId}--inline-description`
const blockDescriptionId = `${itemId}--block-description`
const inactiveWarningId = inactive && !showInactiveIndicator ? `${itemId}--warning-message` : undefined
const validRole = listRole === 'listbox' || listRole === 'menu' || listRole === 'list' || inactive
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm using this conditional as a backwards compatible escape hatch. If an ActionList is using one of these roles, treat it as a list, or something that carries similar semantics. This includes Menu, where can retain the <li> semantics. If developers need to utilize this component as a list they can do so by providing the proper ARIA role.

Copy link
Member

Choose a reason for hiding this comment

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

I'd like to understand the reason of having list role in this condition 🤔

When I think about the semantics

<ul role='menu'>
  <li role='menuitem'></li>
  <li role='menuitem'></li>
</ul>

or

<ul role='listbox'>
  <li role='option'></li> 
  <li role='option'></li>
</ul>

The element that user interacts is the li element. So keeping the current semantics and keeping the onSelect item on the li element make sense to me.

However, when I think about the list, I imagine a list of action items and the semantics in my head is looking like this;

<ul role='list'>
  <li role='listitem'>
    <button>action 1</button>
  </li>
  <li role='listitem'>
      <button>action 2</button>
  </li>
</ul>

In this case, user will interact with the button so we will need to make sure the proper ARIA attributes and onSelect element is triggered on the button element not the list.

So I'd like to understand why we would like to keep the semantics same for the list role as well. Also as far as I know, when there is no role is specified on the <ul> element, it renders as list. So I am a bit confused and would like to understand why we are treating the list role as same as listbox and menu

Let me know if I misunderstood anything here or anything I can clarify on my questions. Thank you 💖

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the write-up! I agree with your comment on role="list". My idea for including role="list" here was as an escape hatch. Using role="list" means that the markup will purely be a list, e.g.

<ul role="list"> <!-- ActionList rendered markup -->
  <li>Item 1</li> 
  <li>Item 2</li>
</ul>

In addition, I added role="list" here to improve our backwards compatibility, as it would allow developers to retain the same semantics as before, if their use case was valid. A good example of this is with NavList. In this component we utilize ActionList.Item as a button (as="button"), and then we add our own custom <li>.

<Box as="li" sx={{ ... }}>
  <ActionList.Item as="button">{ .... }</ActionList.Item>
  <div>{subNav}</div>
</Box>

This will render the following markup:

<li> <!-- Box as="li" -->
  <button>Item 1</button> <!-- ActionList.Item -->
  <div> <!--- SubNav --->
    <ul>{...}</ul> <!-- SubNav `ActionList` -->
  </div>
</li>

NavList uses <Box as="li"> as a container for both ActionList.Item and SubNav items. This is so that the SubNav is a sibling to the ActionList.item as="button". Without role="list" here, it will default to the new semantics, which is okay, but comes with a few issues:

  • Duplicate list items (<li>) due to both <Box as="li"> and <LiBox> being used.
  • Removing <Box as="li"> will mean that the SubNav content is no longer a sibling to ActionList.Item <button>.

I wasn't entirely sure how to fix the above without making more changes to the API of either NavList or ActionList.item, so I chose the path of least resistance 😅. I'm open to any additional feedback, or changes we could make here.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks so much for the great explanation!

It does makes sense to tweak (include list into the valid roles) the implementation to make sure Navlist works as expected because otherwise we would need some additional changes in the implementation.

Using role="list" means that the markup will purely be a list

This is where I am a little unsure how to communicate between an explicit role="list" vs implicit role="list". Because when there is no role prop is specified on ActionList, it also render the list as list. Based on this, we end up having two "list" lists:

  1. interactive list items - Explicit (role="list" is passed as a prop)
<ul> 
  <li>Item 1</li>
  <li>Item 2</li>
</ul>
  1. List items that have buttons - Implicit (no role prop is passed)
<ul> 
  <li>
    <button>Item 1</button>
  </li>
  <li>
    <button>Item 2</button>
  </li>
</ul>

I am not sure why/where we would need the 1. option except supporting existing use cases which is NavList and CustomImagesList plus open source use cases if any.

CustomImagesList looks like a static list, they might be using ActionList just for styling maybe? 🤷🏻‍♀️

All considered, I am thinking would it make sense to exclude NavList use case so we don't have to distinguish explicit vs implicit role? Exclude I mean, it could be reading the context of ActionList and if it is NavList we can keep the semantics same? It is very similar to what your original thinking is, it is just a different condition I am suggesting or thinking loudly to be honest 😄

I'll also tag @siddharthkp to hear his suggestion since he has great context on ActionList 🙌🏻

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it could be reading the context of ActionList and if it is NavList we can keep the semantics same

We could definitely try this route! Even if we wanted to have this condition based through a prop, I think it would work the same more or less. I'll dig on this one, but if we can all agree on it then I'm fine with losing role="list".

Copy link
Member

Choose a reason for hiding this comment

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

This is where I am a little unsure how to communicate between an explicit role="list" vs implicit role="list".

This is where I got confused as well! 😅

I need to poke around this a bit more to understand it better. I'll be back 😇

Copy link
Member

Choose a reason for hiding this comment

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

Do we have any updates on this escape hatch solution? 👀

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have an update! I replaced role="list" to utilize context. This means that we remove role="list" as an escape hatch both internally and externally. Instead, we'll rely on context to retain the same semantics in NavList.

@github-actions github-actions bot temporarily deployed to storybook-preview-4272 June 12, 2024 13:05 Inactive
@khiga8 khiga8 mentioned this pull request Jun 12, 2024
13 tasks
@github-actions github-actions bot temporarily deployed to storybook-preview-4272 June 13, 2024 13:07 Inactive
Copy link
Member

@broccolinisoup broccolinisoup left a comment

Choose a reason for hiding this comment

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

Sorry for stalling this! I think this PR overall is in a good shape and because it is under ff and only semantic changes, I think we can start testing with confidence. 🙌🏻

This was a huge work, thanks so much for working on this and your patience with my nit-pick comments and delayed responses 💖

@github-actions github-actions bot temporarily deployed to storybook-preview-4272 June 17, 2024 12:39 Inactive
@TylerJDev TylerJDev added this pull request to the merge queue Jun 18, 2024
Merged via the queue into main with commit 3c467ef Jun 18, 2024
30 checks passed
@TylerJDev TylerJDev deleted the tylerjdev/action-list-a11y-fixes branch June 18, 2024 21:11
@primer primer bot mentioned this pull request Jun 18, 2024
@siddharthkp siddharthkp added integration-tests: passing Changes in this PR do NOT 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 Aug 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integration-tests: passing Changes in this PR do NOT cause breaking changes in gh/gh
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants