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.

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

fix: ignore invalid and allow redundant role in aria-allowed-role #1118

Merged
merged 9 commits into from
Sep 7, 2018
104 changes: 59 additions & 45 deletions lib/commons/aria/get-element-unallowed-roles.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,46 @@
/* global aria */

/**
* Returns all roles applicable to element in a list
*
* @method getRoleSegments
* @private
* @param {Element} node
* @returns {Array} Roles list or empty list
*/

function getRoleSegments(node) {
let roles = [];

if (!node) {
return roles;
}

if (node.hasAttribute('role')) {
const nodeRoles = axe.utils.tokenList(
node.getAttribute('role').toLowerCase()
);
roles = roles.concat(nodeRoles);
}

if (node.hasAttributeNS('http://www.idpf.org/2007/ops', 'type')) {
const epubRoles = axe.utils
.tokenList(
node
.getAttributeNS('http://www.idpf.org/2007/ops', 'type')
.toLowerCase()
)
.map(role => `doc-${role}`);

roles = roles.concat(epubRoles);
}

// filter invalid roles
roles = roles.filter(role => axe.commons.aria.isValidRole(role));

return roles;
}

/**
* gets all unallowed roles for a given node
* @method getElementUnallowedRoles
Expand All @@ -9,38 +51,8 @@
*/
aria.getElementUnallowedRoles = function getElementUnallowedRoles(
node,
allowImplicit
allowImplicit = true
) {
/**
* Get roles applied to a given node
* @param {HTMLElement} node HTMLElement
* @return {Array<String>} return an array of roles applied to the node, if no roles, return an empty array.
*/
// TODO: not moving this to outer namespace yet, work with wilco to see overlap with his PR(WIP) - aria.getRole
function getRoleSegments(node) {
let roles = [];
if (!node) {
return roles;
}
if (node.hasAttribute('role')) {
const nodeRoles = axe.utils.tokenList(
node.getAttribute('role').toLowerCase()
);
roles = roles.concat(nodeRoles);
}
if (node.hasAttributeNS('http://www.idpf.org/2007/ops', 'type')) {
const epubRoles = axe.utils
.tokenList(
node
.getAttributeNS('http://www.idpf.org/2007/ops', 'type')
.toLowerCase()
)
.map(role => `doc-${role}`);
roles = roles.concat(epubRoles);
}
return roles;
}

const tagName = node.nodeName.toUpperCase();

// by pass custom elements
Expand All @@ -53,24 +65,26 @@ aria.getElementUnallowedRoles = function getElementUnallowedRoles(

// stores all roles that are not allowed for a specific element most often an element only has one explicit role
const unallowedRoles = roleSegments.filter(role => {
if (!axe.commons.aria.isValidRole(role)) {
// do not check made-up/ fake roles
// if role and implicit role are same, when allowImplicit: true
// ignore as it is a redundant role
if (allowImplicit && role === implicitRole) {
return false;
}

// check if an implicit role may be set explicit following a setting
if (!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' &&
axe.utils.matchesSelector(node, 'table[role="grid"] > tr')
)
) {
return true;
}
// Edge case:
// setting implicit role row on tr element is allowed when child of table[role='grid']
if (
!allowImplicit &&
!(
role === 'row' &&
tagName === 'TR' &&
axe.utils.matchesSelector(node, 'table[role="grid"] > tr')
)
) {
return true;
}

// check if role is allowed on element
if (!aria.isAriaRoleAllowedOnElement(node, role)) {
return true;
}
Expand Down
1 change: 1 addition & 0 deletions lib/commons/aria/get-role.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ aria.getRole = function getRole(
}
return aria.isValidRole(role, { allowAbstract: abstracts });
});

const explicitRole = validRoles[0];

// Get the implicit role, if permitted
Expand Down
41 changes: 31 additions & 10 deletions lib/commons/aria/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -420,7 +420,15 @@ lookupTable.role = {
},
nameFrom: ['author'],
context: null,
unsupported: false
unsupported: false,
allowedElements: [
{
tagName: 'INPUT',
attributes: {
TYPE: 'TEXT'
}
}
]
},
command: {
nameFrom: ['author'],
Expand Down Expand Up @@ -1687,7 +1695,15 @@ lookupTable.role = {
nameFrom: ['author'],
context: null,
implicit: ['input[type="search"]'],
unsupported: false
unsupported: false,
allowedElements: [
Copy link
Contributor

Choose a reason for hiding this comment

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

General question: How did we miss this, and how do we know we're not missing more stuff this time around?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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:

  1. Misinterpretation.
  2. 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.
  3. 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.

{
tagName: 'INPUT',
attributes: {
TYPE: 'TEXT'
}
}
]
},
section: {
nameFrom: ['author', 'contents'],
Expand Down Expand Up @@ -1756,7 +1772,15 @@ lookupTable.role = {
nameFrom: ['author'],
context: null,
implicit: ['input[type="number"]'],
unsupported: false
unsupported: false,
allowedElements: [
{
tagName: 'INPUT',
attributes: {
TYPE: 'TEXT'
}
}
]
},
status: {
type: 'widget',
Expand Down Expand Up @@ -2148,13 +2172,6 @@ lookupTable.elementsAllowedNoRole = [
TYPE: 'TEL'
}
},
{
tagName: 'INPUT',
condition: elementConditions.CANNOT_HAVE_LIST_ATTRIBUTE,
attributes: {
TYPE: 'TEXT'
}
},
{
tagName: 'INPUT',
attributes: {
Expand Down Expand Up @@ -2368,6 +2385,10 @@ lookupTable.evaluateRoleForElement = {
return out;
case 'radio':
return role === 'menuitemradio';
case 'text':
return (
role === 'combobox' || role === 'searchbox' || role === 'spinbutton'
);
default:
return false;
}
Expand Down
7 changes: 7 additions & 0 deletions lib/rules/aria-allowed-role-matches.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
return (
axe.commons.aria.getRole(node, {
noImplicit: true,
dpub: true,
fallback: true
}) !== null
);
1 change: 1 addition & 0 deletions lib/rules/aria-allowed-role.json
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
"id": "aria-allowed-role",
"excludeHidden": false,
"selector": "[role]",
"matches": "aria-allowed-role-matches.js",
"tags": [
"cat.aria",
"best-practice"
Expand Down
49 changes: 33 additions & 16 deletions test/checks/aria/aria-allowed-role.js
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,37 @@ describe('aria-allowed-role', function() {
);
});

it('returns false when MENU has type context', function() {
it('returns true when INPUT type is text with role combobox', function() {
var node = document.createElement('input');
node.setAttribute('type', 'text');
node.setAttribute('role', 'combobox');
fixture.appendChild(node);
assert.isTrue(
checks['aria-allowed-role'].evaluate.call(checkContext, node)
);
});

it('returns true when INPUT type is text with role spinbutton', function() {
var node = document.createElement('input');
node.setAttribute('type', 'text');
node.setAttribute('role', 'spinbutton');
fixture.appendChild(node);
assert.isTrue(
checks['aria-allowed-role'].evaluate.call(checkContext, node)
);
});

it('returns true when INPUT type is text with role searchbox', function() {
var node = document.createElement('input');
node.setAttribute('type', 'text');
node.setAttribute('role', 'searchbox');
fixture.appendChild(node);
assert.isTrue(
checks['aria-allowed-role'].evaluate.call(checkContext, node)
);
});

it('returns true when MENU has type context', function() {
var node = document.createElement('menu');
node.setAttribute('type', 'context');
node.setAttribute('role', 'navigation');
Expand Down Expand Up @@ -135,10 +165,8 @@ describe('aria-allowed-role', function() {
node.setAttribute('role', 'link');
node.href = '';
fixture.appendChild(node);
assert.isFalse(
checks['aria-allowed-role'].evaluate.call(checkContext, node)
);
assert.deepEqual(checkContext._data, ['link']);
var actual = checks['aria-allowed-role'].evaluate.call(checkContext, node);
assert.isTrue(actual);
});

it('returns true <img> with a non-empty alt', function() {
Expand All @@ -159,17 +187,6 @@ describe('aria-allowed-role', function() {
assert.isFalse(
checks['aria-allowed-role'].evaluate.call(checkContext, node)
);
// assert.deepEqual(checkContext._data, ['presentation']);
});

it('should not allow a <link> with a href to have any invalid role', function() {
var node = document.createElement('link');
node.setAttribute('role', 'invalid-role');
node.href = '\\example.com';
fixture.appendChild(node);
assert.isTrue(
checks['aria-allowed-role'].evaluate.call(checkContext, node)
);
});

it('should allow <select> without a multiple and size attribute to have a menu role', function() {
Expand Down
18 changes: 18 additions & 0 deletions test/commons/aria/get-element-unallowed-roles.js
Original file line number Diff line number Diff line change
Expand Up @@ -52,4 +52,22 @@ describe('aria.getElementUnallowedRoles', function() {
var actual = axe.commons.aria.getElementUnallowedRoles(node);
assert.isEmpty(actual);
});

it('returns false when role is implicit and allowImplicit is true (default)', function() {
var node = document.createElement('input');
var role = 'textbox';
node.setAttribute('role', role);
var actual = axe.commons.aria.getElementUnallowedRoles(node, true);
assert.isEmpty(actual);
});

it('returns false with implicit role of row for TR when allowImplicit is set to false via options', function() {
var node = document.createElement('table');
node.setAttribute('role', 'grid');
var row = document.createElement('tr');
row.setAttribute('role', 'row');
var actual = axe.commons.aria.getElementUnallowedRoles(row, false);
assert.isNotEmpty(actual);
assert.include(actual, 'row');
});
});
12 changes: 10 additions & 2 deletions test/integration/rules/aria-allowed-role/aria-allowed-role.html
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
<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>
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of removing, I suggest you put in a valid role.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Added it back again. This diff only watches this line, so comment is not going stale.

<div id='pass-div-has-any-role' role='contentinfo'></div>
<div id="pass-div-valid-role" role="link">ok</div>
<ol id="pass-ol-valid-role" role="directory"></ol>
<nav id="pass-nav-role-doc-index" role="doc-index"></nav>
Expand All @@ -27,6 +27,12 @@ <h1 id="pass-h1-role-doc-subtitle" role="doc-subtitle"></h1>
<header id='pass-header-valid-role' role="group"></header>
<footer id='pass-footer-valid-role' role="group"></footer>
<embed id='pass-embed-valid-role' role='img'>
<input type="text" role="textbox" id="pass-input-text-redundant-role"/>
<input type="text" role="textbox combobox" id="pass-input-multiple-roles"/>
<input type="text" role="searchbox invalidrole" id="pass-input-multiple-valid-and-invalid-roles"/>
<input aria-autocomplete="list" aria-label="some label" autocomplete="off" role="searchbox" type="text" aria-expanded="true" id='pass-input-text-role-searchbox'>
<input aria-autocomplete="list" aria-label="some label" autocomplete="off" role="combobox" type="text" aria-expanded="true" id='pass-input-text-role-combobox'>
<input aria-autocomplete="list" aria-label="some label" autocomplete="off" role="spinbutton" type="text" aria-expanded="true" id='pass-input-text-role-spinbutton'>
<input type="image" role="link" id="pass-input-image-valid-role">
<input type="checkbox" role='menuitemcheckbox' id='pass-input-checkbox-valid-role' >
<h1 id='pass-h1-valid-role' role='none'></h1>
Expand All @@ -46,4 +52,6 @@ <h1 id='pass-h1-valid-role' role='none'></h1>
<input type="image" id="fail-input-image-invalid-role" role='doc-afterword'>
<button id="fail-button-role-cell" role='cell'></button>
<aside id='fail-aside-doc-foreword' role='doc-foreword'></aside>
<aside id="fail-aside-role-tab" role='tab'></aside>
<aside id="fail-aside-role-tab" role='tab'></aside>
<button id='fail-button-role-gridcell' role="gridcell" title="IconCheckmark" aria-label="IconCheckmark icon"></button>
<input id='fail-input-role-gridcell-multiple-role' role="gridcell combobox">
12 changes: 10 additions & 2 deletions test/integration/rules/aria-allowed-role/aria-allowed-role.json
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@
["#pass-section-role-doc-bib"],
["#pass-li-role-doc-biblioentry"],
["#pass-aside-doc-example"],
["#pass-div-has-any-role"],
["#pass-div-valid-role"],
["#pass-ol-valid-role"],
["#pass-nav-role-doc-index"],
Expand All @@ -31,6 +30,13 @@
["#pass-header-valid-role"],
["#pass-footer-valid-role"],
["#pass-embed-valid-role"],
["#pass-div-has-any-role"],
["#pass-input-text-redundant-role"],
["#pass-input-multiple-roles"],
["#pass-input-multiple-valid-and-invalid-roles"],
["#pass-input-text-role-searchbox"],
["#pass-input-text-role-combobox"],
["#pass-input-text-role-spinbutton"],
["#pass-input-image-valid-role"],
["#pass-input-checkbox-valid-role"],
["#pass-h1-valid-role"],
Expand All @@ -52,6 +58,8 @@
["#fail-input-image-invalid-role"],
["#fail-button-role-cell"],
["#fail-aside-doc-foreword"],
["#fail-aside-role-tab"]
["#fail-aside-role-tab"],
["#fail-button-role-gridcell"],
["#fail-input-role-gridcell-multiple-role"]
]
}
Loading