feat(new rule): aria-allowed-role #623

lemnis · 2017-11-24T23:57:47Z

As mentioned at #97 (comment).

It's still work in progress, but I want to get some early feedback. My current questions are:

Should the arrays stored somewhere else?
Are the values inside the json set correcty?
Which tags should be used?, I'm rather new to the whole wcag spec.

Things yet to do:

add tests
remove the arrow functions
add more comments to the code

lemnis · 2017-11-29T23:47:49Z

A couple of remarks:

No support yet for SVG elements.

It currently includes DPUB roles.

It validates against the HTML-ARIA spec, but not against the HTML spec. E.g. <button role='option'> is allowed in HTML-ARIA, but not in the current HTML rec.

It currently includes a specific check for <button type='menu'>. This button type was proposed in HTML 5.1 spec but dropped in version 5.2. It never had any browser support. As result when getting the type property, it will return the default value of submit. I could get original value with getAttribute() or removing this specific check. I am leaning to the later.

The menu element has one defined and default value for the type attribute in the HTML 5.1 spec, currently, the element is still mentioned in the living standard but without the type attribute. And the whole element is removed from HTML 5.2. I haven't decided yet what to do with this specific element.

WilcoFiers

Just a few comments while you're working on this.

WilcoFiers · 2017-11-27T10:44:47Z

lib/rules/aria-allowed-role.json

@@ -0,0 +1,18 @@
+{
+  "id": "allowed-role",


Make sure the ID is consistent with the file name. I think aria-role-allowed might be a tiny bit better as an ID?

WilcoFiers · 2017-11-30T09:44:44Z

lib/checks/aria/allowed-role.js

@@ -0,0 +1,207 @@
+/**


I think a lot of the functions and ARIA data should be made into a utility function. Probably on axe.commons.aria.

WilcoFiers · 2017-11-30T09:46:38Z

lib/checks/aria/allowed-role.json

+  "id": "aria-allowed-role",
+  "evaluate": "allowed-role.js",
+  "metadata": {
+    "impact": "critical",


I think this should be minor.

WilcoFiers · 2017-11-30T09:47:28Z

lib/rules/aria-allowed-role.json

+{
+  "id": "aria-allowed-role",
+  "selector": "[role]",
+  "tags": [


This should only contain cat.aria and best-practice. Rules are either a best practice or a WCAG violation. It's never both.

WilcoFiers · 2017-12-03T10:59:47Z

test/checks/aria/allowed-role.js

@@ -0,0 +1,241 @@
+describe('aria-allowed-role', function () {


You don't need this many checks here. The goal of these integration checks is to make sure the logic works. We're less concerned here with exactly which element does what. Those kinds of tests are better done as integration tests. Have a look at the following PR for an example:

https://github.com/dequelabs/axe-core/pull/629/files

WilcoFiers · 2017-12-03T11:05:23Z

lib/checks/aria/allowed-role.js

+var tagName = node.tagName.toLowerCase();
+
+// check if tag is allowed to have a role
+if(elementsHasNoAllowedRoles.indexOf(tagName) > -1) {


We should consider allowing users to whitelist certain elements through options, like we did with some of the other aria checks. Have a look here how this works:

https://github.com/dequelabs/axe-core/blob/develop/lib/checks/aria/valid-attr.js#L11

marcysutton · 2017-12-03T11:48:27Z

lib/checks/aria/allowed-role.js

+ * @return {HTMLElement}         				Parent that matches one of the tagnames
+ */
+function getParentWithTagName(el, tagName) {
+	while (el.parentNode){


There's some inconsistent spacing around the while and if declarations here, as well as the comment blocks. Maybe we should add an automated tool to clean that stuff up. WDYT about adding Prettier in a separate PR, Wilco?

I'm open to experimenting with Prettier. It seemed pretty cool. I don't think we should start with axe-core for that. Maybe a less impactful project to try it out on and adopt it into others if we like it.

marcysutton · 2017-12-03T11:54:39Z

Thanks so much for working on this PR! Really cool to see it coming together.

lemnis · 2017-12-08T13:04:52Z

I'm writing some issues down that I have while implementing this PR, it's mostly my backlog of things that may need a fix in the future or I'm uncertain it's the correct conclusion and needs some further reading.
They could already have been tackled.

general SVG support, e.g., having the implicit graphics-symbol role on circle elements
having the same result while validating attributes who are in a different namespace but who should have a similar logic as its HTML sibling, e.g.
- <section epub:type="dedication"> == <section role="doc-dedication">
- <svg><a xlink:href="#"></a></svg> == <svg><a href="#"></a></svg>
  (both attributes bind its value to the href property)
validating elements against the correct namespace, e.g.
- <svg xmlns:html="http://www.w3.org/1999/xhtml"><html:video></html:video></svg> == <video></video>
- <svg><svg id="foo"></svg></svg> != <html><svg id="foo"></svg></html>
outdated implicit selectors within the lookup tables, e.g. img should be img[alt]:not([alt=""]). But other implicit roles can't be written as an css selector (yet) because they are dependent on the absence of different parents.

…ptions

lemnis · 2017-12-14T11:30:17Z

lib/commons/aria/index.js

@@ -376,8 +376,7 @@ lookupTables.role = {
 			one: ['rowgroup', 'row']
 		},
 		nameFrom: ['author'],
-		context: null,
-		implicit: ['table']
+		context: null


I updated only the implicit roles who needed a update for this new rule to validate.

What was the reason for removing the implicit table?

https://www.w3.org/TR/html-aria/#table - An table element only has an implicit role of table following the current spec.

lemnis · 2017-12-14T11:32:51Z

lib/checks/aria/allowed-role.js

+	// any implicit role is disallowed
+	if (!options.allowImplicit && role === implicitRole) {
+    // edge case: setting implicit role row on tr element is allowed when child of table[role='grid']
+		if (!(role === 'row' && tagName === 'tr' && node.matches('table[role="grid"] :scope'))) {


An edge case where setting a implicit role is allowed, only applicable in the current spec when using <table role='grid'><td role='cell'></td></table>. I don't like the current location of the check...

node.matches() doesn't work in phantomJS. You can use the polyfill in axe.commons.utils.matchesSelector instead though.

lemnis · 2017-12-14T11:38:46Z

I added a option that disables the check on implicit roles, e.g. currently <nav role='navigation'> is disallowed at default. This setting will probably be changed often by the user, because the mentioned html is often recommended for better browser support.

@WilcoFiers

What is opinion about the current setup within the axe.commons.aria ?

Have you any preference how to setup beneath mention cases:

Elements with an implicit role that can only be calculated trough javascript:
- footer
- header
Elements who need some javascript to check if a specific role is allowed.

WilcoFiers · 2017-12-14T12:03:57Z

@lemnis Thank you so very much for the contribution. This is quite a big one, so I'm going to have to set some time aside to properly review it. That may take a little while. The first and most obvious thing is that your tests are failing. Have a look at CircleCI to see what's going wrong. You can run your tests locally by calling grunt test-fast, or if you want to run them individually in the browser, you can use grunt dev.

WilcoFiers · 2018-01-03T13:00:00Z

lib/core/utils/get-tag-name.js

+ * @param  {Element} node element
+ * @return {String}       normalized tagName
+ */
+axe.utils.getTagName = function(node) {


I'm not sure what the value of this is. So far we've solved this by always doing tagName.toUpperCase() for everything. I think we should stick with this.

It is / was mostly specific to svg, rereading some documents my case isn't entirely true anymore.

For an a element in different context it would return:
<a href="#">Hello</a>, tagName will return A and the function will return a
<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink"><a xlink:href="#" target="_top">World</a></svg>, the tagName and function will return a

Colors

For svg gradients you will get:
<svg><linearGradient></linearGradient></svg> tagName and the function will return linearGradient

What I previously thought (svg2 spec):
<svg><solidcolor></solidcolor></svg> tagName and the function will return solidcolor
But it changed to mimic the name conventions in previously used, so when it will be implemented it will be parsed as:
<svg><solidColor></solidColor></svg> tagName and the function willreturn solidColor

I prefer to use the exact name that is shown in the DOM, so an lowercase name for html elements and camelcased names for svg elements.

Also svg2 supports html elements inside an svg, I didn't bother to add the logic as at this moment in time there is no browser support, e.g.

<svg xmlns="http://www.w3.org/2000/svg" xmlns:html="http://www.w3.org/1999/xhtml"> <html:video src="http://example.org/dummyvideo" controls="controls"> <html:p>The video format is not supported by this browser.</html:p> </html:video> </svg>

So (currently) the P element returns an tagName of html:p, but the function should return p.

Conclusion: The function could be removed now, as (I believe) this rule is the only rule that supports svg elements. An type of this function will be needed in the future. A better solution would probably be to check always the tagname with its namespaceURI together. So a elements within svg elements can never mess up the end results.

Good point. I think you're right about the tagName + namespace approach. We shouldn't try to solve that here though. One problem at a time please :). I think we stick to the existing approach for this rule (e.g. .tagName.toUpperCase()) and create a separate issue / PR for this. Can you put together a new issue outlining what the problems are with the current way we're handling tag names?

WilcoFiers · 2018-01-03T13:07:48Z

lib/commons/aria/roles.js

+	if(node.hasAttributeNS('http://www.idpf.org/2007/ops', 'type')) {
+		var rawEpubRoles = node.getAttributeNS('http://www.idpf.org/2007/ops', 'type');
+		var epubRoles = rawEpubRoles.split(' ');
+		roles = roles.concat(epubRoles.forEach((e, i, a) => a[i] = 'doc-' + e));


You could use epubRoles.map(role => 'doc-' + role) here to simplify this a bit.

WilcoFiers · 2018-01-03T13:11:13Z

lib/commons/aria/roles.js

+aria.getRoles = function(node) {
+	var roles = [];
+	if(node.hasAttribute('role')){
+		roles = roles.concat(node.getAttribute('role').split(' '));


This should be more robust. Probably split on any whitespace character, and filter out any empty values that might occur with spaces at the start or end of the string.

WilcoFiers · 2018-01-03T13:22:19Z

lib/checks/aria/allowed-role.js

+	return true;
+}
+
+roles.forEach(function(role) {


If you use roles.reduce() here to create the disallowedRoles, it's much clearer what this loop is supposed to do. You don't have to do the allowed boolean either if you do that. You simply do return disallowed.concat(role); and go on to the next role.

WilcoFiers · 2018-01-03T13:24:36Z

lib/checks/aria/allowed-role.js

+	// any implicit role is disallowed
+	if (!options.allowImplicit && role === implicitRole) {
+    // edge case: setting implicit role row on tr element is allowed when child of table[role='grid']
+		if (!(role === 'row' && tagName === 'tr' && node.matches('table[role="grid"] :scope'))) {


node.matches() doesn't work in phantomJS. You can use the polyfill in axe.commons.utils.matchesSelector instead though.

WilcoFiers · 2018-01-03T13:46:42Z

lib/checks/aria/allowed-role.js

+var disallowedRoles = [];
+
+// check if check on the element is disabled through the options
+if (options.disable && options.disable.indexOf(tagName) > -1) {


I think this should be called options.ignoredTags, so its purpose is clearer.

WilcoFiers · 2018-01-03T13:53:18Z

lib/checks/aria/allowed-role.js

+});
+
+if (disallowedRoles.length) {
+	// this.data(disallowedRoles);


Don't forget to uncomment this. You may want to write some tests to make sure this is working as well.

WilcoFiers · 2018-01-03T14:53:55Z

lib/commons/aria/index.js

@@ -376,8 +376,7 @@ lookupTables.role = {
 			one: ['rowgroup', 'row']
 		},
 		nameFrom: ['author'],
-		context: null,
-		implicit: ['table']
+		context: null


What was the reason for removing the implicit table?

WilcoFiers · 2018-01-03T14:54:09Z

lib/commons/aria/index.js

@@ -388,7 +387,7 @@ lookupTables.role = {
 		owned: null,
 		nameFrom: ['author', 'contents'],
 		context: ['row'],
-		implicit: ['td', 'th']
+		implicit: ['th']


What's the reason for removing td here?

https://www.w3.org/TR/html-aria/#TD - An td never has an implicit role of gridcell following the current spec.

WilcoFiers · 2018-01-03T14:58:23Z

lib/checks/aria/allowed-role.js

+
+  // check within if the element has an allowed subset of roles
+	if (allowed) {
+		allowed = axe.commons.aria.isAllowedRole(tagName, role);


I think isAllowedRole should include the elementCheck[tagName](node, role); call you make. It seems to me the current isAllowedRole doesn't tell the full story, and wouldn't ever be called separate from the element checks. I suggest putting them into their own file, commons/aria/has-allowed-role.js. You get better encapsulation that way.

CLAassistant · 2018-03-25T16:03:49Z

Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ lemnis
❌ lemnisSP
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

jeeyyy · 2018-06-07T07:52:29Z

@lemnis @lemnisSP
Thanks for all the work on this.
I had forked this into a different PR - #945

Hence closing this.

lemnis added 2 commits November 25, 2017 00:47

feat: add rule, allowed-role

1b6a07a

fix(aria-allowed-role): Change ID

cda7c74

lemnis changed the title ~~WIP, new rule: allowed-role~~ WIP, new rule: aria-allowed-role Nov 27, 2017

WilcoFiers reviewed Nov 30, 2017

View reviewed changes

feat(aria-allowed-role): add mocha test + corrected json

bed72af

WilcoFiers reviewed Dec 3, 2017

View reviewed changes

marcysutton reviewed Dec 3, 2017

View reviewed changes

feat(aria-allowed-role): add svg2 support + bug fixes

83e0c0a

lemnis added 3 commits December 11, 2017 15:50

fix(aria-allowed-role): fixes data + stricter implicit roles

07b6e44

test(aria-allowed-role): adds HTMLelement tests

cb95a78

refactor(aria-allowed-role): moved code to axe.commons.aria + added o…

043cfe4

…ptions

lemnis commented Dec 14, 2017

View reviewed changes

WilcoFiers reviewed Jan 3, 2018

View reviewed changes

lemnisSP added 2 commits February 23, 2018 10:27

amend: Merge remote-tracking branch 'upstream/develop' into develop

10822ef

fix(aria-allowed-role): update test

bfda802

amend: develop into fork

79bb6c6

marcysutton mentioned this pull request May 29, 2018

rule: aria-allowed-children #925

Closed

jeeyyy self-assigned this Jun 4, 2018

jeeyyy changed the title ~~WIP, new rule: aria-allowed-role~~ feat(new rule): aria-allowed-role Jun 6, 2018

This was referenced Jun 6, 2018

Refactor: To accommodate namespace for tagName determination (PR#1219) #942

Closed

feat(rule): aria-allowed-role #945

Merged

jeeyyy closed this Jun 7, 2018

stephenmathieson mentioned this pull request Jun 28, 2018

Add Prettier #981

Merged

7 tasks

mrtnvh pushed a commit to mrtnvh/axe-core that referenced this pull request Nov 24, 2023

chore: update chromedriver to use v108 (dequelabs#623)

36584c2

		@@ -0,0 +1,241 @@
		describe('aria-allowed-role', function () {

feat(new rule): aria-allowed-role #623

feat(new rule): aria-allowed-role #623

Conversation

lemnis commented Nov 24, 2017 • edited Loading

lemnis commented Nov 29, 2017

WilcoFiers left a 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

Choose a reason for hiding this comment

marcysutton commented Dec 3, 2017

lemnis commented Dec 8, 2017

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

lemnis commented Dec 14, 2017

WilcoFiers commented Dec 14, 2017

Choose a reason for hiding this comment

lemnis Jan 10, 2018 • edited Loading

Choose a reason for hiding this comment

Colors

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

CLAassistant commented Mar 25, 2018 • edited Loading

jeeyyy commented Jun 7, 2018

lemnis commented Nov 24, 2017 •

edited

Loading

lemnis Jan 10, 2018 •

edited

Loading

CLAassistant commented Mar 25, 2018 •

edited

Loading