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 v1 and v2: Use icon instead of input for multiple selection #1601

Merged
merged 10 commits into from
Nov 19, 2021

Conversation

siddharthkp
Copy link
Member

@siddharthkp siddharthkp commented Nov 11, 2021

Part of https://github.com/github/primer/issues/447
Fixes half of https://github.com/github/primer/issues/228

Nested interactive controls are not announced by screen readers, the accessibility advice here is not use interactive elements for presentation.

  • Replace readonly input type="checkbox" with svg from primer/css
  • Bonus: checkmark consistency across browsers :)

Future - The svg used here has a short life and will get replaced with the Checklist component when it's ready

Screenshots

look pretty ActionList:
image

also shared in AutoComplete:

Screenshot 2021-11-12 at 11 15 34 AM

 

Accessibility violations:

Before:

image

After:

image

Please provide before/after screenshots for any visual changes

Merge checklist

  • NA Added/updated tests
  • NA Added/updated documentation
  • Tested in Chrome
  • Tested in Firefox
  • Tested in Safari
  • Tested in Edge

@siddharthkp siddharthkp requested review from a team and colebemis November 11, 2021 16:03
@changeset-bot
Copy link

changeset-bot bot commented Nov 11, 2021

🦋 Changeset detected

Latest commit: 8319437

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 56.31 KB (+0.52% 🔺)
dist/browser.umd.js 56.7 KB (+0.52% 🔺)

@pksjce
Copy link
Collaborator

pksjce commented Nov 15, 2021

The solution is so interesting! I've never heard of someone using an svg instead of a checkbox for a11y reasons. Why not use the solution proposed in https://github.com/github/primer/issues/228 where they recommended to make the role presentational ? Is that for checkbox browser consistency?

@siddharthkp
Copy link
Member Author

siddharthkp commented Nov 15, 2021

The solution is so interesting!

credit to @langermank for introducing it in primer/css

Why not use the solution proposed in github/primer#228 where they recommended to make the role presentational ?

We are? 😅 The example mentioned in the issue uses an image with role="presentation":

<li role="menuitemcheckbox" aria-checked="true">
  <img src="checked.gif" role="presentation" alt="">
  Sort by Last Modified
</li>

From what I understand, having an interactive element inside a selectable listbox option or menuitem isn't valid. See violations on story

@langermank
Copy link
Contributor

From what I understand, having an interactive element inside a selectable listbox option or menuitem isn't valid. See violations on story

Exactly. input needs to be in a form and ActionList isn't really a form scenario. I attended a popup a11y meeting last week and was able to confirm this with a screen reader user, which was super helpful!

Copy link
Contributor

@mperrotti mperrotti left a comment

Choose a reason for hiding this comment

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

LGTM, and it doesn't break the multiselect behavior in Autocomplete 🙌

@siddharthkp siddharthkp merged commit 250e4b0 into main Nov 19, 2021
@siddharthkp siddharthkp deleted the siddharthkp/actionlist-checkbox-a11y branch November 19, 2021 14:48
@primer-css primer-css mentioned this pull request Nov 19, 2021
This was referenced Nov 22, 2021
@siddharthkp siddharthkp changed the title ActionList: Use icon instead of input for multiple selection ActionList v1 + v2: Use icon instead of input for multiple selection Nov 30, 2021
@siddharthkp siddharthkp changed the title ActionList v1 + v2: Use icon instead of input for multiple selection ActionList v2: Use icon instead of input for multiple selection Nov 30, 2021
@siddharthkp siddharthkp changed the title ActionList v2: Use icon instead of input for multiple selection ActionList v1 and v2: Use icon instead of input for multiple selection Nov 30, 2021
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

4 participants