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(get-role): add presentation role resolution and inheritance #2281

Merged
merged 12 commits into from
Jun 23, 2020
Merged

feat(get-role): add presentation role resolution and inheritance #2281

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
c180e70
feat(get-role): add presentation role resolution and inheritance
straker Jun 8, 2020
c0d9fbd
comment
straker Jun 8, 2020
8cc43b9
add test
straker Jun 8, 2020
e67e77b
fix all but test
straker Jun 11, 2020
b0014ce
merge develop
straker Jun 16, 2020
13a0c17
Update lib/commons/aria/get-role.js
straker Jun 17, 2020
2eb6719
Update lib/commons/aria/get-role.js
straker Jun 17, 2020
3761d7e
Update lib/commons/aria/get-role.js
straker Jun 17, 2020
628ee6e
Update lib/commons/aria/get-role.js
straker Jun 17, 2020
a31f62d
fixes
straker Jun 17, 2020
e1d8250
throw error
straker Jun 17, 2020
35fff93
changes
straker Jun 22, 2020
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
133 changes: 126 additions & 7 deletions lib/commons/aria/get-role.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,115 @@
import getExplicitRole from './get-explicit-role';
import getImplicitRole from './implicit-role';
import lookupTable from './lookup-table';
import isFocusable from '../dom/is-focusable';
import { getNodeFromTree } from '../../core/utils';
import AbstractVirtuaNode from '../../core/base/virtual-node/abstract-virtual-node';

// when an element inherits the presentational role from a parent
// is not defined in the spec, but through testing it seems to be
// when a specific HTML parent relationship is required and that
// parent has `role=presentation`, then the child inherits the
// role (i.e. table, ul, dl). Further testing has shown that
// intermediate elements (such as divs) break this chain only in
// Chrome.
//
// Also, any nested structure chains reset the role (so two nested
// lists with the topmost list role=none will not cause the nested
// list to inherit the role=none).
//
// from Scott O'Hara:
//
// "the expectation for me, in standard html is that element
// structures that require specific parent/child relationships,
// if the parent is set to presentational that should set the
// children to presentational. ala, tables and lists."
// "but outside of those specific constructs, i would not expect
// role=presentation to do anything to child element roles"
const inheritsPresentationChain = {
// valid parent elements, any other element will prevent any
// children from inheriting a presentational role from a valid
// ancestor
td: ['tr'],
th: ['tr'],
tr: ['thead', 'tbody', 'tfoot', 'table'],
thead: ['table'],
tbody: ['table'],
tfoot: ['table'],
table: [],
li: ['ol', 'ul'],
ol: [],
ul: [],
dt: ['dl'],
dd: ['dl'],
dl: []
Copy link
Contributor

Choose a reason for hiding this comment

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

I just looked these up, as far as I can tell this stuff doesn't work with dl, which is good because otherwise we'd have to deal with div being allowed to group dd / dt elements.

Suggested change
dt: ['dl'],
dd: ['dl'],
dl: []

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn't work in Chrome but does in Firefox, Safari/VO, and IE11/JAWS:

image

image

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 had assumed <div> would stop the chain as we've seen in other things, but after testing it does in fact carry through the role inheritance so we'll need to add that.

};

// role presentation inheritance.
// Source: https://www.w3.org/TR/wai-aria-1.1/#conflict_resolution_presentation_none
function getPresentationalAncestorRole(vNode, explicitRoleOptions) {
straker marked this conversation as resolved.
Show resolved Hide resolved
const chain = inheritsPresentationChain[vNode.props.nodeName];

if (!chain) {
return null;
}

const role = getExplicitRole(vNode, explicitRoleOptions);
if (role && !hasConflictResolution(vNode)) {
// an explicit role of anything other than presentational will
// prevent any children from inheriting a presentational role
// from a valid ancestor
return ['presentation', 'none'].includes(role) ? role : null;
}

// if we can't look at the parent then we can't know if the node
// inherits the presentational role or not
if (!vNode.parent) {
throw new ReferenceError(
'Cannot determine role presentational inheritance of a required parent outside the current scope.'
);
}

if (!chain.includes(vNode.parent.props.nodeName)) {
return null;
}

return getPresentationalAncestorRole(vNode.parent);
}

function resolveImplicitRole(vNode, explicitRoleOptions) {
const implicitRole = getImplicitRole(vNode);

if (!implicitRole) {
return null;
}

const presentationalRole = getPresentationalAncestorRole(
vNode,
explicitRoleOptions
);
if (presentationalRole) {
return presentationalRole;
}

return implicitRole;
}

// role conflict resolution
// note: Chrome returns a list with resolved role as "generic"
// instead of as a list
// (e.g. <ul role="none" aria-label><li>hello</li></ul>)
// we will return it as a list as that is the best option.
// Source: https://www.w3.org/TR/wai-aria-1.1/#conflict_resolution_presentation_none
// See also: https://github.com/w3c/aria/issues/1270
function hasConflictResolution(vNode) {
const hasGlobalAria = lookupTable.globalAttributes.some(attr =>
vNode.hasAttr(attr)
);
return hasGlobalAria || isFocusable(vNode.actualNode);
}

/**
* Return the semantic role of an element
* Return the semantic role of an element.
*
* @method getRole
* @memberof axe.commons.aria
Expand All @@ -19,20 +124,34 @@ import AbstractVirtuaNode from '../../core/base/virtual-node/abstract-virtual-no
*
* @deprecated noImplicit option is deprecated. Use aria.getExplicitRole instead.
*/
function getRole(node, { noImplicit, fallback, abstracts, dpub } = {}) {
function getRole(node, { noImplicit, ...explicitRoleOptions } = {}) {
const vNode =
node instanceof AbstractVirtuaNode ? node : getNodeFromTree(node);
if (vNode.props.nodeType !== 1) {
return null;
}
const explicitRole = getExplicitRole(vNode, { fallback, abstracts, dpub });

// Get the implicit role, if permitted
if (!explicitRole && !noImplicit) {
return getImplicitRole(vNode);
const explicitRole = getExplicitRole(vNode, explicitRoleOptions);
const implicitRole = noImplicit
? null
: resolveImplicitRole(vNode, explicitRoleOptions);

if (!explicitRole) {
return implicitRole;
}

if (!['presentation', 'none'].includes(explicitRole)) {
return explicitRole;
}

if (hasConflictResolution(vNode)) {
// return null if there is a conflict resolution but no implicit
// has been set as the explicit role is not the true role
return implicitRole;
straker marked this conversation as resolved.
Show resolved Hide resolved
}

return explicitRole || null;
// role presentation or none and no conflict resolution
return explicitRole;
straker marked this conversation as resolved.
Show resolved Hide resolved
}

export default getRole;
18 changes: 17 additions & 1 deletion lib/commons/aria/lookup-table.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import idrefs from '../dom/idrefs';
import isColumnHeader from '../table/is-column-header';
import isRowHeader from '../table/is-row-header';
import sanitize from '../text/sanitize';
import isFocusable from '../dom/is-focusable';
import { closest } from '../../core/utils';

const isNull = value => value === null;
Expand Down Expand Up @@ -2168,7 +2169,19 @@ lookupTable.implicitHtmlRole = {
},
hr: 'separator',
img: vNode => {
return vNode.hasAttr('alt') && !vNode.attr('alt') ? null : 'img';
// an images role is considered implicitly presentation if the
// alt attribute is empty. But that shouldn't be the case if it
// has global aria attributes or is focusable, so we need to
// override the role back to `img`
// e.g. <img alt="" aria-label="foo"></img>
const emptyAlt = vNode.hasAttr('alt') && !vNode.attr('alt');
const hasGlobalAria = lookupTable.globalAttributes.find(attr =>
vNode.hasAttr(attr)
);

return emptyAlt && !hasGlobalAria && !isFocusable(vNode.actualNode)
? 'presentation'
: 'img';
},
input: vNode => {
// Source: https://www.w3.org/TR/html52/sec-forms.html#suggestions-source-element
Expand Down Expand Up @@ -2202,6 +2215,9 @@ lookupTable.implicitHtmlRole = {
return !suggestionsSourceElement ? 'searchbox' : 'combobox';
}
},
// Note: if an li (or some other elms) do not have a required
// parent, Firefox ignores the implicit semantic role and treats
// it as a generic text.
li: 'listitem',
main: 'main',
math: 'math',
Expand Down
10 changes: 1 addition & 9 deletions lib/commons/text/title-text.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,7 @@
import matches from '../matches/matches';
import getRole from '../aria/get-role';

const alwaysTitleElements = [
'button',
'iframe',
'a[href]',
{
nodeName: 'input',
properties: { type: 'button' }
}
];
const alwaysTitleElements = ['iframe'];
Copy link
Contributor

Choose a reason for hiding this comment

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

This shouldn't be necessary. IsFocusable is getting this wrong. For the purpose of what we're doing here, iframes are focusable areas. Unlike other areas they route focus to the first focusable area inside of it, unless there is none in which case you can clearly see iframes are focusable. Take this example:

<button>hello</button>
<iframe width="100" height="100"></iframe>
<button>world</button>

There is a tab stop between "hello" and "world". It isn't visible, the :focus style doesn't apply, but activeElement is set. Here's where to find it in the HTML spec: https://html.spec.whatwg.org/#bc-focus-ergo-bcc-focus

I think what we need to do is update isFocusable, give it a flag to return true for iframes. Something like this:

const myFrame = document.querySelector('iframe')
isFocusable(myFrame) // false
isFocusable(myFrame, { focusRouters: true }) // 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.

That's fine, but I think that's outside the scope of this pr. We should create another pr that fixes the iframe issue of isFocusable.


/**
* Get title text
Expand Down
17 changes: 7 additions & 10 deletions test/commons/aria/attributes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,16 +33,19 @@ describe('aria.allowedAttr', function() {
'use strict';

var orig;
var origGlobals;
beforeEach(function() {
orig = axe.commons.aria.lookupTable.role;
origGlobals = axe.commons.aria.lookupTable.globalAttributes;
});

afterEach(function() {
axe.commons.aria.lookupTable.role = orig;
axe.commons.aria.lookupTable.globalAttributes = origGlobals;
});

it('should returned the attributes property for the proper role', function() {
var orig = (axe.commons.aria.lookupTable.globalAttributes = ['world']);
axe.commons.aria.lookupTable.globalAttributes = ['world'];
axe.commons.aria.lookupTable.role = {
cats: {
attributes: {
Expand All @@ -52,11 +55,10 @@ describe('aria.allowedAttr', function() {
};

assert.deepEqual(axe.commons.aria.allowedAttr('cats'), ['hello', 'world']);
axe.commons.aria.lookupTable.globalAttributes = orig;
});

it('should also check required attributes', function() {
var orig = (axe.commons.aria.lookupTable.globalAttributes = ['world']);
axe.commons.aria.lookupTable.globalAttributes = ['world'];
axe.commons.aria.lookupTable.role = {
cats: {
attributes: {
Expand All @@ -71,18 +73,13 @@ describe('aria.allowedAttr', function() {
'world',
'hello'
]);
axe.commons.aria.lookupTable.globalAttributes = orig;
});

it('should return an array with globally allowed attributes', function() {
var result,
orig = (axe.commons.aria.lookupTable.globalAttributes = ['world']);

axe.commons.aria.lookupTable.globalAttributes = ['world'];
axe.commons.aria.lookupTable.role = {};
result = axe.commons.aria.allowedAttr('cats');

assert.deepEqual(result, ['world']);
axe.commons.aria.lookupTable.globalAttributes = orig;
assert.deepEqual(axe.commons.aria.allowedAttr('cats'), ['world']);
});
});

Expand Down
Loading