-
Notifications
You must be signed in to change notification settings - Fork 15
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
Use aria-checked for AssignTesterDropdown #1097
Use aria-checked for AssignTesterDropdown #1097
Conversation
@alflennik Upon further inspection of the Asana task, I see that the proposed pattern is https://www.w3.org/WAI/ARIA/apg/patterns/menu-button/examples/menu-button-actions-active-descendant/. Is the intention to change the dropdown in favor of that? |
@howard-e On reading the issue, it seems that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@howard-e On reading the issue, it seems that
aria-checked
should suffice, but the Asana task says that we should change the pattern to a menu button. What is the right course of action?
@evmiguel looking at the issue and in reviewing this, the linked comment seems unrelated. However, a role change is still required here. The <Dropdown.Item>
role here should be menuitemcheckbox
, in order for aria-checked
to work as expected since this requires a checkable menuitem. A relevant APG example on that.
Additionally, the aria-hidden
should be removed from the <span>
which I've commented inline.
testerIsAssigned | ||
? 'checked' | ||
: 'unchecked' | ||
}`}</span> | ||
<span | ||
aria-hidden="true" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This aria-hidden="true"
should be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@howard-e I've added the menuitemcheckbox
role and removed aria-hidden
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks for addressing the feedback
Issues addressed: * #1105, addresses w3c/aria-at#1070 * #1053, addresses w3c/aria-practices#2971 * #1097, addresses #977 * #1095, addresses #991 * #1093, addresses #934 * #1000, addresses #818 * #1089, addresses #992 * #1067, addresses #993 * #1056, addresses w3c/wai-aria-practices#212 --------- Co-authored-by: alflennik <alflennik@users.noreply.github.com> Co-authored-by: Paul Clue <67766160+Paul-Clue@users.noreply.github.com> Co-authored-by: Mx Corey Frang <corey@bocoup.com> Co-authored-by: Mx. Corey Frang <gnarf37@gmail.com> Co-authored-by: Erika Miguel <erika@bocoup.com> Co-authored-by: Mike Pennisi <mike@mikepennisi.com>
This PR addresses #977. It removes the screen reader
div
in favor ofaria-checked
for theAssignTesterDropdown
.