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

ActionList: Correct semantics for Groups #1598

Merged
merged 6 commits into from
Nov 22, 2021

Conversation

siddharthkp
Copy link
Member

@siddharthkp siddharthkp commented Nov 11, 2021

checklist:

  • ActionList.Group create a ul inside a li
  • The title of the Group is used as the label for the ul.
  • The section Header is for visual users only. It's hidden for screen readers.
  • Group can accept role

 

This is the generated HTML, formatted for legibility:

image

<ul aria-label="Select reviewers">
  <li>
    <div role="presentation" aria-hidden="true">
      <span id="react-aria-1">Suggestions</span>
    </div>
    <ul aria-labelledby="react-aria-1" role="listbox">
      <li
        role="option"
        aria-selected="true"
        aria-labelledby="react-aria-2"
        aria-describedby="react-aria-3 react-aria-4"
      >
        <span>
          <input type="checkbox" tabindex="-1" readonly aria-readonly="false" checked />
        </span>
        <span class="Box-nv15kw-0 iBXgEg">
          <img src="https://github.com/pksjce.png" />
        </span>
        <div data-component="ActionList.Item--DividerContainer">
          <div>
            <span id="react-aria-2">pksjce</span>
            <div id="react-aria-3" title="Pavithra Kodmad">Pavithra Kodmad</div>
          </div>
          <span id="react-aria-4">Recently edited these files</span>
        </div>
      </li>
      <li>....</li>
    </ul>
  </li>
  <li>
    <div role="presentation" aria-hidden="true">
      <span id="react-aria-5">Everyone</span>
    </div>
    <ul aria-labelledby="react-aria-5" role="listbox">
      <li>...</li>
      <li>...</li>
      <li>...</li>
    </ul>
  </li>

Merge checklist

@changeset-bot
Copy link

changeset-bot bot commented Nov 11, 2021

🦋 Changeset detected

Latest commit: 40481f3

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

This PR includes changesets to release 1 package
Name Type
@primer/components 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
Copy link
Contributor

github-actions bot commented Nov 11, 2021

size-limit report 📦

Path Size
dist/browser.esm.js 55.96 KB (0%)
dist/browser.umd.js 56.37 KB (0%)

/**
* The ARIA role describing the function of the list inside `Group` component. `listbox` or `menu` are a common values.
*/
role?: AriaRole
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: Given there's a limited number of suitable roles for this component, can we help the user decision along by providing a union of those compatible types?

Copy link
Member Author

@siddharthkp siddharthkp Nov 22, 2021

Choose a reason for hiding this comment

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

I'm a bit hesitant because it's heavily context dependent so it's hard to create good types for it.

  1. I'm hoping to set these automatically when you use our menu components like ActionMenu or SelectPanel and increase their coverage.
  2. For building your menu story (common escape hatch in memex), I'm a bit hesitant about adding "hints" that send you the wrong way - classic a11y story, better to not say anything than say the wrong thing

I think I'll come around to this feeling more confident when we replace the escape hatches in memex with the right menu components (ActionMenu/Autocomplete/SelectPanel/etc)

Copy link
Contributor

@rezrah rezrah left a comment

Choose a reason for hiding this comment

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

Look good!

@siddharthkp siddharthkp merged commit 3bb895f into main Nov 22, 2021
@siddharthkp siddharthkp deleted the siddharth/actionlist-groups-a11y branch November 22, 2021 16:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants