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

Jump to bottom

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

Merged
merged 9 commits into from
Sep 7, 2018
Merged

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

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
119fe33
fix: ignore invalid and allow redundant role in aria-allowed-role
jeeyyy Aug 31, 2018
fceb0d7
test: aria allowed roles, tests for invalid and redundant roles
jeeyyy Sep 2, 2018
4ea825a
fix: allowed combobox, searchbox, spinbutton role on type input text
jeeyyy Sep 3, 2018
401d41e
test: aria allowed role unit tests
jeeyyy Sep 3, 2018
2086d07
test: get role tests for segments
jeeyyy Sep 3, 2018
dcacb51
refactor: review based code changes
jeeyyy Sep 5, 2018
2170563
test: add more integration tests
jeeyyy Sep 5, 2018
39e7784
refactor: based on review
jeeyyy Sep 6, 2018
a54843e
test: add tests for aria-allowed-role
jeeyyy Sep 6, 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
48 changes: 12 additions & 36 deletions lib/commons/aria/get-element-unallowed-roles.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
/* global aria */

/**
* gets all unallowed roles for a given node
* @method getElementUnallowedRoles
Expand All @@ -11,53 +12,22 @@ aria.getElementUnallowedRoles = function getElementUnallowedRoles(
node,
allowImplicit
) {
/**
* 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
if (!axe.utils.isHtmlElement(node)) {
return [];
}

const roleSegments = getRoleSegments(node);
const roleSegments = axe.commons.aria.getRole(node, {
segments: true,
fallback: true,
dpub: true
});
const implicitRole = axe.commons.aria.implicitRole(node);

// 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
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']
Expand All @@ -71,6 +41,12 @@ aria.getElementUnallowedRoles = function getElementUnallowedRoles(
return true;
}
}

// if role and implicit role are same, it is redundant - ignore
if (role === implicitRole) {
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 confused about why you're doing this for a second time. Line 28 already tests for implicit roles, correctly taking the allowImplicit option into consideration.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right, that flow path, only handles the edge case for table>tr[role='row']. I shall refactor.

return false;
}

if (!aria.isAriaRoleAllowedOnElement(node, role)) {
return true;
}
Expand Down
8 changes: 7 additions & 1 deletion lib/commons/aria/get-role.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,12 @@
* @param {boolean} options.fallback Allow fallback roles
* @param {boolean} options.abstracts Allow role to be abstract
* @param {boolean} options.dpub Allow role to be any (valid) doc-* roles
* @param {boolean} options.segments Allows getting full list of roles applied
* @returns {string|null} Role or null
*/
aria.getRole = function getRole(
node,
{ noImplicit, fallback, abstracts, dpub } = {}
{ noImplicit, fallback, abstracts, dpub, segments } = {}
) {
const roleAttr = (node.getAttribute('role') || '').trim().toLowerCase();
const roleList = fallback ? axe.utils.tokenList(roleAttr) : [roleAttr];
Expand All @@ -28,6 +29,11 @@ aria.getRole = function getRole(
}
return aria.isValidRole(role, { allowAbstract: abstracts });
});

if (segments) {
return validRoles;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

It's not at all obvious that segments wouldn't return the implicit role. I'm also not a fan of having this turn into a thing that can return either strings or arrays. You're trying to do two things with the same function. I suggest you keep the getRoleSegments function around, and have getRole call it to get the validRoles value instead.


const explicitRole = validRoles[0];

// Get the implicit role, if permitted
Expand Down
41 changes: 31 additions & 10 deletions lib/commons/aria/index.js
View file
Open in desktop
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
18 changes: 18 additions & 0 deletions lib/rules/aria-allowed-role-matches.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
const { isValidRole, getRole } = axe.commons.aria;

// get role(s) as array(segments), with fallback and dpub
const roleSegments = getRole(node, {
segments: true,
fallback: true,
dpub: true
});

// filter invalid roles
const validRoles = roleSegments.filter(role => isValidRole(role));
Copy link
Contributor

Choose a reason for hiding this comment

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

getRole already filters out invalid roles, doesn't it? Couldn't you just do this:

return getRole(node, { noImplicit: true, dpub: true }) !== null


// if no valid roles are applied, ignore
if (!validRoles.length) {
return false;
}

return true;
1 change: 1 addition & 0 deletions lib/rules/aria-allowed-role.json
View file
Open in desktop
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
47 changes: 32 additions & 15 deletions test/checks/aria/aria-allowed-role.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,36 @@ describe('aria-allowed-role', 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 false when MENU has type context', function() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like the test description needs to be inverted to match the new expected result.

var node = document.createElement('menu');
node.setAttribute('type', 'context');
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
15 changes: 15 additions & 0 deletions test/commons/aria/get-role.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,21 @@ describe('aria.getRole', function() {
});
});

describe('segments', function() {
var options = {
fallback: true,
dpub: true,
segments: true
};

it('returns all roles from element', function() {
var node = document.createElement('input');
node.setAttribute('role', 'textbox combobox');
var actual = aria.getRole(node, options);
assert.deepEqual(actual, ['textbox', 'combobox']);
});
});

describe('fallback', function() {
it('returns the first valid item in the list', function() {
var node = document.createElement('div');
Expand Down
5 changes: 4 additions & 1 deletion test/integration/rules/aria-allowed-role/aria-allowed-role.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@
<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-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 +26,10 @@ <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 aria-autocomplete="list" aria-label="some label" autocomplete="off" role="searchbox" type="text" aria-expanded="true" aria-owns="autocomplete-0" id='pass-input-text-role-searchbox'>
<input aria-autocomplete="list" aria-label="some label" autocomplete="off" role="combobox" type="text" aria-expanded="true" aria-owns="autocomplete-0" id='pass-input-text-role-combobox'>
<input aria-autocomplete="list" aria-label="some label" autocomplete="off" role="spinbutton" type="text" aria-expanded="true" aria-owns="autocomplete-0" 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 Down
5 changes: 4 additions & 1 deletion test/integration/rules/aria-allowed-role/aria-allowed-role.json
View file
Open in desktop
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,10 @@
["#pass-header-valid-role"],
["#pass-footer-valid-role"],
["#pass-embed-valid-role"],
["#pass-input-text-redundant-role"],
["#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 Down
45 changes: 45 additions & 0 deletions test/rule-matches/aria-allowed-role-matches.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
describe('aria-allowed-role-matches', function() {
'use strict';

var fixture = document.getElementById('fixture');
var rule;

beforeEach(function() {
rule = axe._audit.rules.find(function(rule) {
return rule.id === 'aria-allowed-role';
});
});

afterEach(function() {
fixture.innerHTML = '';
});

it('is a function', function() {
assert.isFunction(rule.matches);
});

it('return false (no matches) for 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.isFalse(rule.matches(node));
});

it('return true for input with redundant role', function() {
var node = document.createElement('input');
node.setAttribute('type', 'text');
node.setAttribute('role', 'textbox');
node.href = '\\example.com';
fixture.appendChild(node);
assert.isTrue(rule.matches(node));
});

it('return true for element with valid role', function() {
var node = document.createElement('ol');
node.setAttribute('role', 'listbox');
node.href = '\\example.com';
fixture.appendChild(node);
assert.isTrue(rule.matches(node));
});
});