Skip to content
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.

Sign up for GitHub

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

feat: ARIA supported checks #1254

Merged
merged 8 commits into from
Dec 14, 2018
Merged

feat: ARIA supported checks #1254

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
e1462c5
chore: move unsupportedrole test into correct directory
Nov 21, 2018
def0a8f
feat: add unsupported ARIA attribute check
Nov 21, 2018
1af5f93
WIP: start messing with aria-allowed-attr matches
Nov 21, 2018
145b52a
fix: move unsupportedattr check to `none`
Dec 11, 2018
15eaf47
fix: remove extra axe.log
Dec 11, 2018
d2b634c
Merge remote-tracking branch 'origin/develop' into aria-supported-checks
Dec 11, 2018
c41bd38
Merge branch 'develop' into aria-supported-checks
jeeyyy Dec 12, 2018
35344b5
Merge branch 'develop' into aria-supported-checks
WilcoFiers Dec 14, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion doc/aria-supported.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,4 +18,4 @@ For a detailed description about how accessibility support is decided, see [How
| -------------------- | ---------------- |
| aria-describedat | No |
| aria-details | No |
| aria-roledescription | No |
| aria-roledescription | No |
154 changes: 77 additions & 77 deletions doc/rule-descriptions.md
View file
Open in desktop

Large diffs are not rendered by default.

16 changes: 16 additions & 0 deletions lib/checks/aria/unsupportedattr.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
let unsupported = Array.from(node.attributes)
.filter(candidate => {
// filter out unsupported attributes
return axe.commons.aria.validateAttr(candidate.name, {
flagUnsupported: true
});
})
.map(candidate => {
return candidate.name.toString();
});

if (unsupported.length) {
this.data(unsupported);
return true;
}
return false;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure the logic works here. Shouldn't you return true when this passes? I would also think that this needs to be in all instead of any in the rule. That decision may be why you used this strange logic.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@WilcoFiers the aria-roles unsupported stuff works on none. I would actually think that should be the case here as well.

11 changes: 11 additions & 0 deletions lib/checks/aria/unsupportedattr.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
{
"id": "aria-unsupported-attr",
"evaluate": "unsupportedattr.js",
"metadata": {
"impact": "critical",
"messages": {
"pass": "ARIA attribute is supported",
"fail": "ARIA attribute is not widely supported in screen readers and assistive technologies: {{~it.data:value}} {{=value}}{{~}}"
}
}
}
2 changes: 1 addition & 1 deletion lib/checks/aria/unsupportedrole.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
"impact": "critical",
"messages": {
"pass": "ARIA role is supported",
"fail": "The role used is not widely supported in assistive technologies"
"fail": "The role used is not widely supported in screen readers and assistive technologies: {{~it.data:value}} {{=value}}{{~}}"
}
}
}
12 changes: 7 additions & 5 deletions lib/commons/aria/attributes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@
* @return {Array}
*/
aria.requiredAttr = function(role) {
'use strict';
var roles = aria.lookupTable.role[role],
attr = roles && roles.attributes && roles.attributes.required;
return attr || [];
Expand All @@ -24,7 +23,6 @@ aria.requiredAttr = function(role) {
* @return {Array}
*/
aria.allowedAttr = function(role) {
'use strict';
var roles = aria.lookupTable.role[role],
attr = (roles && roles.attributes && roles.attributes.allowed) || [],
requiredAttr =
Expand All @@ -39,9 +37,13 @@ aria.allowedAttr = function(role) {
* @memberof axe.commons.aria
* @instance
* @param {String} att The attribute name
* @param {Object} options Use `flagUnsupported: true` to report unsupported attributes
* @return {Boolean}
*/
aria.validateAttr = function(att) {
'use strict';
return !!aria.lookupTable.attributes[att];
aria.validateAttr = function(att, { flagUnsupported = false } = {}) {
const attrDefinition = aria.lookupTable.attributes[att];
if (flagUnsupported && attrDefinition) {
return !!attrDefinition.unsupported;
}
return !!attrDefinition;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is a little difficult to read, with all the logic operations going on. This may be simpler:

const attrDefinition = aria.lookupTable.attributes[att];
if (flagUnsupported && attrDefinition) {
  return !!attrDefinition.unsupported
}
return !!attrDefinition

};
148 changes: 102 additions & 46 deletions lib/commons/aria/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,177 +10,233 @@ var aria = (commons.aria = {}),
lookupTable.attributes = {
'aria-activedescendant': {
type: 'idref',
allowEmpty: true
allowEmpty: true,
unsupported: false
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know we did this for all the roles too, but do we really need to set these explicitly? I think that's the virtue of using unsupported over supported, you don't have to specify it to be falsey. I'd have a slight preference for only setting it if it's true.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leaving them off and using falsey values instead sounds fine to me!

},
'aria-atomic': {
type: 'boolean',
values: ['true', 'false']
values: ['true', 'false'],
unsupported: false
},
'aria-autocomplete': {
type: 'nmtoken',
values: ['inline', 'list', 'both', 'none']
values: ['inline', 'list', 'both', 'none'],
unsupported: false
},
'aria-busy': {
type: 'boolean',
values: ['true', 'false']
values: ['true', 'false'],
unsupported: false
},
'aria-checked': {
type: 'nmtoken',
values: ['true', 'false', 'mixed', 'undefined']
values: ['true', 'false', 'mixed', 'undefined'],
unsupported: false
},
'aria-colcount': {
type: 'int'
type: 'int',
unsupported: false
},
'aria-colindex': {
type: 'int'
type: 'int',
unsupported: false
},
'aria-colspan': {
type: 'int'
type: 'int',
unsupported: false
},
'aria-controls': {
type: 'idrefs',
allowEmpty: true
allowEmpty: true,
unsupported: false
},
'aria-current': {
type: 'nmtoken',
allowEmpty: true,
values: ['page', 'step', 'location', 'date', 'time', 'true', 'false']
values: ['page', 'step', 'location', 'date', 'time', 'true', 'false'],
unsupported: false
},
'aria-describedby': {
type: 'idrefs',
allowEmpty: true
allowEmpty: true,
unsupported: false
},
'aria-describedat': {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is one of the attributes that was proposed for ARIA but never made it into the spec. Did you mean to add a non-standard attribute in here? I can see a use case for including attributes that were at one point proposed but never made it into the spec. We would have to be explicit about it though, and we'd have to put some sort of "unstandardised" flag on it. That's probably out of scope for this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can leave it off, I only added it because it was mentioned in the aria-supported diff. But if we did include it, if someone tried to use it they'd see it was unsupported. I like the idea of an "unstandardized" flag in here.

unsupported: true,
unstandardized: true
},
'aria-details': {
unsupported: true
},
'aria-disabled': {
type: 'boolean',
values: ['true', 'false']
values: ['true', 'false'],
unsupported: false
},
'aria-dropeffect': {
type: 'nmtokens',
values: ['copy', 'move', 'reference', 'execute', 'popup', 'none']
values: ['copy', 'move', 'reference', 'execute', 'popup', 'none'],
unsupported: false
},
'aria-errormessage': {
type: 'idref',
allowEmpty: true
allowEmpty: true,
unsupported: false
},
'aria-expanded': {
type: 'nmtoken',
values: ['true', 'false', 'undefined']
values: ['true', 'false', 'undefined'],
unsupported: false
},
'aria-flowto': {
type: 'idrefs',
allowEmpty: true
allowEmpty: true,
unsupported: false
},
'aria-grabbed': {
type: 'nmtoken',
values: ['true', 'false', 'undefined']
values: ['true', 'false', 'undefined'],
unsupported: false
},
'aria-haspopup': {
type: 'nmtoken',
allowEmpty: true,
values: ['true', 'false', 'menu', 'listbox', 'tree', 'grid', 'dialog']
values: ['true', 'false', 'menu', 'listbox', 'tree', 'grid', 'dialog'],
unsupported: false
},
'aria-hidden': {
type: 'boolean',
values: ['true', 'false']
values: ['true', 'false'],
unsupported: false
},
'aria-invalid': {
type: 'nmtoken',
allowEmpty: true,
values: ['true', 'false', 'spelling', 'grammar']
values: ['true', 'false', 'spelling', 'grammar'],
unsupported: false
},
'aria-keyshortcuts': {
type: 'string',
allowEmpty: true
allowEmpty: true,
unsupported: false
},
'aria-label': {
type: 'string',
allowEmpty: true
allowEmpty: true,
unsupported: false
},
'aria-labelledby': {
type: 'idrefs',
allowEmpty: true
allowEmpty: true,
unsupported: false
},
'aria-level': {
type: 'int'
type: 'int',
unsupported: false
},
'aria-live': {
type: 'nmtoken',
values: ['off', 'polite', 'assertive']
values: ['off', 'polite', 'assertive'],
unsupported: false
},
'aria-modal': {
type: 'boolean',
values: ['true', 'false']
values: ['true', 'false'],
unsupported: false
},
'aria-multiline': {
type: 'boolean',
values: ['true', 'false']
values: ['true', 'false'],
unsupported: false
},
'aria-multiselectable': {
type: 'boolean',
values: ['true', 'false']
values: ['true', 'false'],
unsupported: false
},
'aria-orientation': {
type: 'nmtoken',
values: ['horizontal', 'vertical']
values: ['horizontal', 'vertical'],
unsupported: false
},
'aria-owns': {
type: 'idrefs',
allowEmpty: true
allowEmpty: true,
unsupported: false
},
'aria-placeholder': {
type: 'string',
allowEmpty: true
allowEmpty: true,
unsupported: false
},
'aria-posinset': {
type: 'int'
type: 'int',
unsupported: false
},
'aria-pressed': {
type: 'nmtoken',
values: ['true', 'false', 'mixed', 'undefined']
values: ['true', 'false', 'mixed', 'undefined'],
unsupported: false
},
'aria-readonly': {
type: 'boolean',
values: ['true', 'false']
values: ['true', 'false'],
unsupported: false
},
'aria-relevant': {
type: 'nmtokens',
values: ['additions', 'removals', 'text', 'all']
values: ['additions', 'removals', 'text', 'all'],
unsupported: false
},
'aria-required': {
type: 'boolean',
values: ['true', 'false']
values: ['true', 'false'],
unsupported: false
},
'aria-roledescription': {
unsupported: true
},
'aria-rowcount': {
type: 'int'
type: 'int',
unsupported: false
},
'aria-rowindex': {
type: 'int'
type: 'int',
unsupported: false
},
'aria-rowspan': {
type: 'int'
type: 'int',
unsupported: false
},
'aria-selected': {
type: 'nmtoken',
values: ['true', 'false', 'undefined']
values: ['true', 'false', 'undefined'],
unsupported: false
},
'aria-setsize': {
type: 'int'
type: 'int',
unsupported: false
},
'aria-sort': {
type: 'nmtoken',
values: ['ascending', 'descending', 'other', 'none']
values: ['ascending', 'descending', 'other', 'none'],
unsupported: false
},
'aria-valuemax': {
type: 'decimal'
type: 'decimal',
unsupported: false
},
'aria-valuemin': {
type: 'decimal'
type: 'decimal',
unsupported: false
},
'aria-valuenow': {
type: 'decimal'
type: 'decimal',
unsupported: false
},
'aria-valuetext': {
type: 'string'
type: 'string',
unsupported: false
}
};

Expand Down
19 changes: 6 additions & 13 deletions lib/rules/aria-allowed-attr-matches.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,16 +1,9 @@
var role = node.getAttribute('role');
if (!role) {
role = axe.commons.aria.implicitRole(node);
}
var allowed = axe.commons.aria.allowedAttr(role);
if (role && allowed) {
var aria = /^aria-/;
if (node.hasAttributes()) {
var attrs = node.attributes;
for (var i = 0, l = attrs.length; i < l; i++) {
if (aria.test(attrs[i].name)) {
return true;
}
const aria = /^aria-/;
if (node.hasAttributes()) {
let attrs = node.attributes;
for (let i = 0, l = attrs.length; i < l; i++) {
if (aria.test(attrs[i].name)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, should have noticed this in my first review. It seems like you're not filtering out elements with only invalid ARIA attributes:

<input aria-invalidattr="foo" />

This element shouldn't be applicable to this rule. It's a pretty minor bug, so I'll leave it for you to decide if you want to bother with it. The fix is to use axe.commons.aria.validateAttr in the if statement here.

return true;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can simplify this quite a bit. There's also no reason for this hasAttributes thing. There is no performance benefit to using DOM has methods. Only use them if you're not interested in the actual content of the attribute.

return Array.from(node.attributes)
  .some(({ name }) => /^aria-/.test(name))

}
}
}
Expand Down
2 changes: 1 addition & 1 deletion lib/rules/aria-allowed-attr.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,5 +8,5 @@
},
"all": [],
"any": ["aria-allowed-attr"],
"none": []
"none": ["aria-unsupported-attr"]
}
Loading