-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
report(accessibility): make dropdown match ARIA action menu button pattern #9433
Conversation
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.
I think this is a step in the right direction, I just need some project guidance on the best way to implement UI controls (in JS) or try to keep state in the DOM.
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.
IDK about the accessibility stuff, but I commented on what I do know about :)
Quick updateI am working on feedback and will have changes ready for another round of review early next week. |
I have implemented the requested feedback, PTAL @connorjclark |
ping @connorjclark, @patrickhulce |
PTAL @paulirish |
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 @johnemau!
onToolsDropDownKeydown(e) { | ||
const el = /** @type {?HTMLElement} */ (e.target); | ||
|
||
switch (e.code) { |
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.
could reorder and fall thru these cases
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.
Good idea, I will get that in tomorrow.
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.
I took a closer look at this and was not able to reorder the cases to allow fall thrus, the issue I hit was ArrowUp/Down require a start element and Home/End should not have a start element.
Let me know if I missed something, thanks!
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 impl looks fine in general.
after we land this tomorrow would you be willing to do a small followup which refactors the toolsButton/toolsDropdown into a separate class?
the class could remain here in the same file.. it'd just be nice to separate things a bit, now that this has grown.
cheers.
Thanks for the review!
You got it! I have the refactor started in a branch and will open the PR once this is merged. |
@paulirish, as requested: #9564 |
Summary
This improves the accessibility of the report dropdown by adding ARIA attributes and interactive behavior in-line with the WCAG menu button pattern, specifically the navigation menu button.
Screenshot of NVDA speech view + the dropdown
Screenshot of button accessibility tree
Screenshot of menu item accessibility tree
Gif of keyboard interactions
This will meet WCAG success criteria 1.3.1 Info and Relationship and 4.1.2 Name, Role, Value
Related Issues/PRs
#9183