feat: ARIA supported checks #1254

marcysutton · 2018-11-21T23:07:31Z

This branch adds a new check to report unsupported ARIA attributes to the user. We have to make a decision on how to modify the rule to support it, which you can find discussed in #918.

Closes issue: #918

Reviewer checks

Required fields, to be filled out by PR reviewer(s)

Follows the commit message policy, appropriate for next version
Has documentation updated, a DU ticket, or requires no documentation change
Includes new tests, or was unnecessary
Code is reviewed for security by: @WilcoFiers

References #918

Logic is duplicated in check

WilcoFiers · 2018-11-24T11:15:52Z

lib/checks/aria/unsupportedattr.js

+let unsupported;
+if (node.hasAttributes()) {
+	unsupported = [].slice
+		.call(node.attributes)


You could use `Array.from(node.attributes) to simplify this a bit.

WilcoFiers · 2018-11-24T11:20:37Z

lib/checks/aria/unsupportedattr.js

+		.call(node.attributes)
+		.filter(candidate => {
+			// skip non-ARIA attributes
+			if (/aria-/.test(candidate.name)) {


You probably don't need to do this if statement. Just rely on aria.validateAttr to sort that out for you. That way you also always return. It's a little nicer to read a filter if it always returns a boolean.

WilcoFiers · 2018-11-24T12:39:40Z

lib/commons/aria/attributes.js

+	if (!attrDefinition || (flagUnsupported && isAttrUnsupported)) {
+		return false;
+	}
+	return !!attrDefinition;


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

WilcoFiers · 2018-11-24T12:41:54Z

lib/commons/aria/index.js

@@ -10,177 +10,232 @@ var aria = (commons.aria = {}),
 lookupTable.attributes = {
 	'aria-activedescendant': {
 		type: 'idref',
-		allowEmpty: true
+		allowEmpty: true,
+		unsupported: false


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.

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

WilcoFiers · 2018-11-24T12:48:49Z

lib/commons/aria/index.js

+		allowEmpty: true,
+		unsupported: false
+	},
+	'aria-describedat': {


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.

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.

WilcoFiers · 2018-11-24T12:55:19Z

lib/rules/aria-allowed-attr-matches.js

+	let attrs = node.attributes;
+	for (let i = 0, l = attrs.length; i < l; i++) {
+		if (aria.test(attrs[i].name)) {
+			return true;


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))

WilcoFiers · 2018-11-24T13:02:17Z

lib/rules/aria-allowed-attr.json

@@ -7,6 +7,6 @@
 		"help": "Elements must only use allowed ARIA attributes"
 	},
 	"all": [],
-	"any": ["aria-allowed-attr"],
+	"any": ["aria-allowed-attr", "aria-unsupported-attr"],


What's the reasoning behind adding this to aria-allowed-attr instead of to aria-valid-attr? I haven't quite figured out which place I think is best, so I'd like to know your reasoning for it.

It's a really fine line. I went for "allowed" because technically these attributes are valid, but we're recommending they not be used because of support purposes. Both aria-allowed-attr and aria-valid-attr use the same commons method to validate, except aria-allowed-attr has some more logic to check the role. The new unsupported check could go in either rule, really.

I still believe that the correct rule for this behavior is aria-allowed-attr rather than aria-valid-attr, so we can get the correct messaging. I'm going to move forward with the change to the matches function so we can keep this rolling.

WilcoFiers · 2018-11-24T13:17:58Z

lib/checks/aria/unsupportedattr.js

+	this.data(unsupported);
+	return true;
+}
+return false;


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.

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

marcysutton · 2018-12-11T01:54:50Z

This is ready for another review.

Test failure on CircleCI is unrelated:

>> URL: http://localhost:9876/test/integration/full/landmark-contentinfo-is-top-level/landmark-contentinfo-is-top-level-fail.html
>> Describe: landmark-contentinfo-is-top-level test fail
>> it "before all" hook
>> Error: Timeout of 10000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves.

Warning: Task "test-webdriver:chrome-mobile" failed.� Use --force to continue.

marcysutton · 2018-12-11T01:55:56Z

test/checks/media/description.js

@@ -40,7 +40,6 @@ describe('description', function() {
 			shadow.innerHTML = '<video><slot></slot></video>';

 			var checkArgs = checkSetup(node, {}, 'video');
-			axe.log(checkArgs);


This commit should have test in the scope, and is unrelated to this PR. But it kept irritating me when running tests.

PR updated

WilcoFiers

Only small stuff left.

WilcoFiers · 2018-12-12T16:38:48Z

lib/rules/aria-allowed-attr-matches.js

+if (node.hasAttributes()) {
+	let attrs = node.attributes;
+	for (let i = 0, l = attrs.length; i < l; i++) {
+		if (aria.test(attrs[i].name)) {


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.

Marcy Sutton added 3 commits November 21, 2018 13:37

chore: move unsupportedrole test into correct directory

e1462c5

feat: add unsupported ARIA attribute check

def0a8f

References #918

WIP: start messing with aria-allowed-attr matches

1af5f93

Logic is duplicated in check

marcysutton requested a review from WilcoFiers November 21, 2018 23:07

marcysutton requested a review from a team as a code owner November 21, 2018 23:07

marcysutton mentioned this pull request Nov 21, 2018

Add ATSupported checks to ARIA rules #918

Closed

WilcoFiers previously requested changes Nov 24, 2018

View reviewed changes

marcysutton added the support label Nov 27, 2018

Marcy Sutton added 3 commits December 10, 2018 16:52

fix: move unsupportedattr check to none

145b52a

fix: remove extra axe.log

15eaf47

Merge remote-tracking branch 'origin/develop' into aria-supported-checks

d2b634c

marcysutton changed the title ~~WIP: ARIA supported checks~~ ARIA supported checks Dec 11, 2018

marcysutton commented Dec 11, 2018

View reviewed changes

Merge branch 'develop' into aria-supported-checks

c41bd38

WilcoFiers requested changes Dec 12, 2018

View reviewed changes

WilcoFiers approved these changes Dec 12, 2018

View reviewed changes

marcysutton changed the title ~~ARIA supported checks~~ feat: ARIA supported checks Dec 12, 2018

Merge branch 'develop' into aria-supported-checks

35344b5

WilcoFiers merged commit 51a18a8 into develop Dec 14, 2018

jeeyyy deleted the aria-supported-checks branch December 17, 2018 16:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: ARIA supported checks #1254

feat: ARIA supported checks #1254

marcysutton commented Nov 21, 2018 •

edited by WilcoFiers

Loading

WilcoFiers Nov 24, 2018

WilcoFiers Nov 24, 2018

WilcoFiers Nov 24, 2018

WilcoFiers Nov 24, 2018

marcysutton Nov 27, 2018

WilcoFiers Nov 24, 2018

marcysutton Nov 27, 2018

WilcoFiers Nov 24, 2018

WilcoFiers Nov 24, 2018

marcysutton Nov 26, 2018

marcysutton Dec 10, 2018

WilcoFiers Nov 24, 2018

marcysutton Dec 10, 2018

marcysutton commented Dec 11, 2018 •

edited

Loading

marcysutton Dec 11, 2018

WilcoFiers left a comment

WilcoFiers Dec 12, 2018

feat: ARIA supported checks #1254

feat: ARIA supported checks #1254

Conversation

marcysutton commented Nov 21, 2018 • edited by WilcoFiers Loading

Reviewer checks

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcysutton commented Dec 11, 2018 • edited Loading

Choose a reason for hiding this comment

WilcoFiers left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcysutton commented Nov 21, 2018 •

edited by WilcoFiers

Loading

marcysutton commented Dec 11, 2018 •

edited

Loading