fix: Ignore abstracts in determining element roles #1015
Conversation
| @@ -1,18 +1,21 @@ | |||
| const parent = axe.commons.dom.getComposedParent(node); | |||
| if (!parent) { | |||
| return false; | |||
| // Can only happen with detached DOM nodes and roots: | |||
There was a problem hiding this comment.
should we do similar check for dlitem ?
There was a problem hiding this comment.
Could have. If I wasn't already in the process of changing these functions I would.
| (ALLOWED_TAGS.includes(parentTagName) && (!parentRole || isListRole)) || | ||
| isListRole | ||
| ); | ||
| return ['UL', 'OL'].includes(parentTagName); |
There was a problem hiding this comment.
readable code FTW
lib/rules/heading-matches.js
Outdated
| } | ||
| console.log(explicitRoles); |
WilcoFiers
force-pushed
the
isValidRole-abstracts
branch
from
9bf2e97
to
e66fce0
Compare
WilcoFiers
force-pushed
the
isValidRole-abstracts
branch
from
e66fce0
to
1af6088
Compare
| return true; | ||
| } | ||
|
|
||
| if (parentRole && axe.commons.aria.isValidRole(parentRole)) { |
There was a problem hiding this comment.
Shouldn't this check for ARIA roles that aren't list to return false, like the code did before?
if (parentRole && !axe.commons.aria.isValidRole(parentRole)) {
There was a problem hiding this comment.
This fixes the bug I mentioned in the description:
This PR fixes a bug that was introduced in a PR that landed earlier today. The PR caused invalid roles to fail the list rules, rather than having it ignore them. There's no ticket for it.
| 'use strict'; | ||
| if (aria.lookupTable.role[role]) { | ||
| return true; | ||
| aria.isValidRole = function(role, { allowAbstract } = {}) { |
There was a problem hiding this comment.
The description of this issue says disallowAbstract, while this says allowAbstract. Shouldn't they be the same?
There was a problem hiding this comment.
Gd point, forgot to update the description. I flipped it around after talking to Dylan about what we can and can't do with commons in minor releases. The more sensible default is to not consider abstract roles valid, so the default allowAbstract: false is used.
doc/rule-descriptions.md
Outdated
| | landmark-no-duplicate-banner | Ensures the document has no more than one banner landmark | Moderate | cat.semantics, best-practice | true | | ||
| | landmark-no-duplicate-contentinfo | Ensures the document has no more than one contentinfo landmark | Moderate | cat.semantics, best-practice | true | | ||
| | landmark-one-main | Ensures a navigation point to the primary content of the page. If the page contains iframes, each iframe should contain either no main landmarks or just one | Moderate | cat.semantics, best-practice | true | | ||
| | landmark-banner-is-top-level | Ensures the banner landmark is at top level | Moderate | cat.semantics, best-practice | true | |
There was a problem hiding this comment.
Minor: move these documentation updates to separate PR
| @@ -1 +1,3 @@ | |||
| return !axe.commons.aria.isValidRole(node.getAttribute('role')); | |||
| return !axe.commons.aria.isValidRole(node.getAttribute('role'), { | |||
| allowAbstract: true | |||
There was a problem hiding this comment.
The word allow is throwing me off here, like abstract roles are being "permitted" rather than "included for evaluation". Maybe that's just me though?
Edit: I suppose that's because we are in fact allowing abstracts, as they are ignored by the accessibility API. Carry on.
Added
allowAbstracttoaria.isValidRoleand put it to use in a few places. Couple of things to note:aria.isValidRoleis used, I looked at those and I didn't think they needed this. But you might want to check it out for yourself.Closes #1014
Reviewer checks
Required fields, to be filled out by PR reviewer(s)