fix: Consider explicit role when pretty printing roles #568
Conversation
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 17bb655:
|
Codecov Report
@@ Coverage Diff @@
## master #568 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 23 23
Lines 459 462 +3
Branches 113 114 +1
=========================================
+ Hits 459 462 +3
Continue to review full report at Codecov.
|
eps1lon
changed the title
| let roles = [] | ||
| // TODO: This violates html-aria which does not allow any role on every element | ||
| if (node.hasAttribute('role')) { | ||
| roles = node.getAttribute('role').split(' ').slice(0, 1) |
There was a problem hiding this comment.
Can an element have multiple roles? If so, is there a reason we're only getting the first one here?
There was a problem hiding this comment.
An element can only have a single role. We only consider arrays right now for historic reasons. At some point aria-query will return a single role but this requires more work that isn't actually blocking this PR.
If you pass in multiple roles then those are considered fallback roles. We allow querying them but haven't decided yet if we want to include it in the error message. It gets quite complicate since you have to branch one more time.
I don't think this is a concern of this PR. aria-query returning multiple roles or queryFallback not being considered can be solved independently.
| @@ -134,7 +134,13 @@ function getRoles(container, {hidden = false} = {}) { | |||
| return hidden === false ? isInaccessible(element) === false : true | |||
| }) | |||
| .reduce((acc, node) => { | |||
| const roles = getImplicitAriaRoles(node) | |||
| let roles = [] | |||
| // TODO: This violates html-aria which does not allow any role on every element | |||
There was a problem hiding this comment.
I'd love to deal with this TODO before merging if it's not a huge effort.
There was a problem hiding this comment.
Oh, just noticed what you said about this. Maybe rather than TODO this should link to some explanation for why we're doing this.
There was a problem hiding this comment.
It is a TODO though. The way we currently handle it is wrong. But this can be fixed in a separate PR so that we're able to revert if multiple roles are required still (also better for changelog).
|
🎉 This PR is included in version 7.5.3 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
What:
Consider explicit roles first when pretty printing roles.
Why:
getRolesprinted a different role than whatqueryAllByRolecould query. Closes #553How:
Check for existence of
roleattribute first and use the first role from that list. I haven't put much thought into fallback roles but we can discuss it when it becomes an issue.Technically this isn't correct. html-aria disallows (some) roles for certain HTML elements. But we didn't consider this for
queryAllByRoleeither and it isn't clear if we should restrict it (e.g. chrome's a11y pane does not).Checklist:
[ ] Documentation added to thedocs site
[ ] I've prepared a PR for types targetingDefinitelyTyped