fix: ignore invalid and allow redundant role in aria-allowed-role #1118
Conversation
jeeyyy
changed the title
|
@dylanb - kindly review, believe it will be good to release a patch this week with these fixes. |
| checks['aria-allowed-role'].evaluate.call(checkContext, node) | ||
| ); | ||
| }); | ||
|
|
||
| it('returns false when MENU has type context', function() { |
There was a problem hiding this comment.
Looks like the test description needs to be inverted to match the new expected result.
lib/commons/aria/get-role.js
Outdated
|
|
||
| if (segments) { | ||
| return validRoles; | ||
| } |
There was a problem hiding this comment.
It's not at all obvious that segments wouldn't return the implicit role. I'm also not a fan of having this turn into a thing that can return either strings or arrays. You're trying to do two things with the same function. I suggest you keep the getRoleSegments function around, and have getRole call it to get the validRoles value instead.
| @@ -1687,7 +1695,15 @@ lookupTable.role = { | |||
| nameFrom: ['author'], | |||
| context: null, | |||
| implicit: ['input[type="search"]'], | |||
| unsupported: false | |||
| unsupported: false, | |||
| allowedElements: [ | |||
There was a problem hiding this comment.
General question: How did we miss this, and how do we know we're not missing more stuff this time around?
There was a problem hiding this comment.
This is the holy grail source of truth - https://www.w3.org/TR/html-aria/ to construct these mappings.
There are 3 caveats with this:
- Misinterpretation.
- Conformance between ARIA1.0 and ARIA1.1, that was the case with aria-allowed-role with custom autocomplete (combobox + listbox, aria 1.0 vs aria 1.1) (PR #1118) #1116. There were multiple ways to use a role. 1.1 mandates usage of role on parent, but backwards compatibility with 1.0 means role can be set on said element itself.
- When https://www.w3.org/TR/html-aria/ is updated, we need a handle to confirm that things have not been changed, and adapt to new changes. Perhaps can watch/ github hook for the same and streamline the mapping.
To answer your specific Q, we missed this case due to an overlap of 1 and 2 above.
I am going through the table once more to ensure the mappings are right.
| }); | ||
|
|
||
| // filter invalid roles | ||
| const validRoles = roleSegments.filter(role => isValidRole(role)); |
There was a problem hiding this comment.
getRole already filters out invalid roles, doesn't it? Couldn't you just do this:
return getRole(node, { noImplicit: true, dpub: true }) !== null| @@ -7,7 +7,6 @@ | |||
| <section id="pass-section-role-doc-bib" role="doc-bibliography"></section> | |||
| <ul><li id='pass-li-role-doc-biblioentry' role="doc-biblioentry"></li></ul> | |||
| <aside id='pass-aside-doc-example' role='doc-example'></aside> | |||
| <div id='pass-div-has-any-role' role='divAnyRoleEvenInvalid'></div> | |||
There was a problem hiding this comment.
Instead of removing, I suggest you put in a valid role.
There was a problem hiding this comment.
Done. Added it back again. This diff only watches this line, so comment is not going stale.
| @@ -71,6 +37,12 @@ aria.getElementUnallowedRoles = function getElementUnallowedRoles( | |||
| return true; | |||
| } | |||
| } | |||
|
|
|||
| // if role and implicit role are same, it is redundant - ignore | |||
| if (role === implicitRole) { | |||
There was a problem hiding this comment.
I'm confused about why you're doing this for a second time. Line 28 already tests for implicit roles, correctly taking the allowImplicit option into consideration.
There was a problem hiding this comment.
You are right, that flow path, only handles the edge case for table>tr[role='row']. I shall refactor.
| * @returns {Array} Roles list or empty list | ||
| */ | ||
|
|
||
| aria.getRoleSegments = function getRoleSegments(node) { |
There was a problem hiding this comment.
Either add tests for this, or don't move this into a commons. My preference would be the latter. I think you've made this PR much larger than it needed to be for the fix.
There was a problem hiding this comment.
There were tests (dcacb51#diff-c01d2c9028bf4f6a16bace64d8ce59daR1) indeed . They were at the bottom of the PR. I needed to use getRoleSegments in 2 places, one within the matches, the other within the check, hence the abstraction to commons.
But believe I could use getRole in matches.
Will remove aria.getRoleSegments
|
@WilcoFiers - reviewer checks were not filled up. Doing it on your behalf. |
This PR tackles fix for ignoring invalid roles and allowing redundant roles in aria-allowed role. It also handles backwards compatibility for ARIA 1 vs 1.1 for certain roles like
combobox,spinbuttonandsearchbox.Closes issue:
aria-allowed-roleissues with axe-core>=3.1.0 (PR #1118) #1110Reviewer checks
Required fields, to be filled out by PR reviewer(s)