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(2859) presentation role conflict #2948

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion doc/standards-object.md
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ The [`htmlElms`](../lib/standards/html-elms.js) object defines valid HTML elemen

- `aria-allowed-attr` - Checks if the attribute can be used on the element from the `noAriaAttrs` property.
- `aria-allowed-role` - Checks if the role can be used on the HTML element from the `allowedRoles` property.
- `aria-required-attrs` - Checks if any required attrs are defied implicitly on the element from the `implicitAttrs` property.
- `aria-required-attrs` - Checks if any required attrs are defined implicitly on the element from the `implicitAttrs` property.

### Structure

Expand Down
28 changes: 16 additions & 12 deletions lib/checks/aria/aria-prohibited-attr-evaluate.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,36 +27,40 @@ import standards from '../../standards';
* @return {Boolean} True if the element uses any prohibited ARIA attributes. False otherwise.
*/
function ariaProhibitedAttrEvaluate(node, options = {}, virtualNode) {
const { elementsAllowedAriaLabel = [] } = options;
const prohibitedList = listProhibitedAttrs(virtualNode, elementsAllowedAriaLabel);

const extraElementsAllowedAriaLabel = options.elementsAllowedAriaLabel || [];

const prohibitedList = listProhibitedAttrs(
virtualNode,
extraElementsAllowedAriaLabel
);

const prohibited = prohibitedList.filter(attrName => {
if (!virtualNode.attrNames.includes(attrName)) {
return false;
}
return sanitize(virtualNode.attr(attrName)) !== ''
return sanitize(virtualNode.attr(attrName)) !== '';
});

if (prohibited.length === 0) {
return false;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Odd, looks like prettier isn't running on my machine :/. I'll have to look at that.


this.data(prohibited);
const hasTextContent = sanitize(subtreeText(virtualNode)) !== ''
const hasTextContent = sanitize(subtreeText(virtualNode)) !== '';
// Don't fail if there is text content to announce
return hasTextContent ? undefined : true;
}

function listProhibitedAttrs(virtualNode, elementsAllowedAriaLabel) {
const role = getRole(virtualNode);
const roleSpec = standards.ariaRoles[role]
const role = getRole(virtualNode, { chromium: true });
const roleSpec = standards.ariaRoles[role];
if (roleSpec) {
return roleSpec.prohibitedAttrs || [];
}
const { nodeName } = virtualNode.props
if (elementsAllowedAriaLabel.includes(nodeName)) {
return []

const { nodeName } = virtualNode.props;
if (!!role || elementsAllowedAriaLabel.includes(nodeName)) {
return [];
}
return ['aria-label', 'aria-labelledby'];
}
Expand Down
15 changes: 1 addition & 14 deletions lib/checks/aria/aria-prohibited-attr.json
Original file line number Diff line number Diff line change
Expand Up @@ -2,20 +2,7 @@
"id": "aria-prohibited-attr",
"evaluate": "aria-prohibited-attr-evaluate",
"options": {
"elementsAllowedAriaLabel": [
"audio",
"applet",
"canvas",
"dl",
"embed",
"iframe",
"input",
"label",
"meter",
"object",
"svg",
"video"
]
"elementsAllowedAriaLabel": ["applet", "input"]
},
"metadata": {
"impact": "serious",
Expand Down
15 changes: 9 additions & 6 deletions lib/commons/aria/get-role.js
Original file line number Diff line number Diff line change
Expand Up @@ -82,8 +82,10 @@ function getInheritedRole(vNode, explicitRoleOptions) {
return getInheritedRole(vNode.parent, explicitRoleOptions);
}

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

if (!implicitRole) {
return null;
Expand Down Expand Up @@ -118,17 +120,17 @@ function hasConflictResolution(vNode) {
* @returns {string|null} Role or null
* @deprecated noImplicit option is deprecated. Use aria.getExplicitRole instead.
*/
function resolveRole(node, { noImplicit, ...explicitRoleOptions } = {}) {
function resolveRole(node, { noImplicit, ...roleOptions } = {}) {
const vNode =
node instanceof AbstractVirtuaNode ? node : getNodeFromTree(node);
if (vNode.props.nodeType !== 1) {
return null;
}

const explicitRole = getExplicitRole(vNode, explicitRoleOptions);
const explicitRole = getExplicitRole(vNode, roleOptions);

if (!explicitRole) {
return noImplicit ? null : resolveImplicitRole(vNode, explicitRoleOptions);
return noImplicit ? null : resolveImplicitRole(vNode, roleOptions);
}

if (!['presentation', 'none'].includes(explicitRole)) {
Expand All @@ -138,7 +140,7 @@ function resolveRole(node, { noImplicit, ...explicitRoleOptions } = {}) {
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 noImplicit ? null : resolveImplicitRole(vNode, explicitRoleOptions);
return noImplicit ? null : resolveImplicitRole(vNode, roleOptions);
}

// role presentation or none and no conflict resolution
Expand All @@ -158,6 +160,7 @@ function resolveRole(node, { noImplicit, ...explicitRoleOptions } = {}) {
* @param {boolean} options.abstracts Allow role to be abstract
* @param {boolean} options.dpub Allow role to be any (valid) doc-* roles
* @param {boolean} options.noPresentational return null if role is presentation or none
* @param {boolean} options.chromium Include implicit roles from chromium-based browsers in role result
* @returns {string|null} Role or null
*
* @deprecated noImplicit option is deprecated. Use aria.getExplicitRole instead.
Expand Down
16 changes: 6 additions & 10 deletions lib/commons/aria/implicit-role.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import implicitHtmlRoles from '../standards/implicit-html-roles';
import { getNodeFromTree } from '../../core/utils';
import getElementSpec from '../standards/get-element-spec';
import AbstractVirtuaNode from '../../core/base/virtual-node/abstract-virtual-node';

/**
Expand All @@ -10,7 +11,7 @@ import AbstractVirtuaNode from '../../core/base/virtual-node/abstract-virtual-no
* @param {HTMLElement|VirtualNode} node The node to test
* @return {Mixed} Either the role or `null` if there is none
*/
function implicitRole(node) {
function implicitRole(node, { chromium } = {}) {
const vNode =
node instanceof AbstractVirtuaNode ? node : getNodeFromTree(node);
node = vNode.actualNode;
Expand All @@ -25,24 +26,19 @@ function implicitRole(node) {
);
}

// until we have proper implicit role lookups for svgs we will
// avoid giving them one
if (node && node.namespaceURI === 'http://www.w3.org/2000/svg') {
return null;
}
Comment on lines -28 to -32
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you remove this?

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 looked like an unnecessary check - we don't have implicit role lookups for svg, so it will return null without this check based on the lines that follow.

If we left it in, we'd have to add another check here for whether to use the chromium roles, and return the svg's implicit chromium role or else null. That's the same logic as is below.


const nodeName = vNode.props.nodeName;
const role = implicitHtmlRoles[nodeName];

if (!role) {
return null;
if (!role && chromium) {
const { chromiumRole } = getElementSpec(vNode);
return chromiumRole || null;
}

if (typeof role === 'function') {
return role(vNode);
}

return role;
return role || null;
}

export default implicitRole;
1 change: 1 addition & 0 deletions lib/commons/aria/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ export { default as arialabelledbyText } from './arialabelledby-text';
export { default as getAccessibleRefs } from './get-accessible-refs';
export { default as getElementUnallowedRoles } from './get-element-unallowed-roles';
export { default as getExplicitRole } from './get-explicit-role';
export { default as getImplicitRole } from './implicit-role';
export { default as getOwnedVirtual } from './get-owned-virtual';
export { default as getRoleType } from './get-role-type';
export { default as getRole } from './get-role';
Expand Down
2 changes: 2 additions & 0 deletions lib/core/base/metadata-function-map.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import ariaValidAttrEvaluate from '../../checks/aria/aria-valid-attr-evaluate';
import ariaValidAttrValueEvaluate from '../../checks/aria/aria-valid-attr-value-evaluate';
import fallbackroleEvaluate from '../../checks/aria/fallbackrole-evaluate';
import hasGlobalAriaAttributeEvaluate from '../../checks/aria/has-global-aria-attribute-evaluate';
import hasImplicitChromiumRoleMatches from '../../rules/has-implicit-chromium-role-matches';
import hasWidgetRoleEvaluate from '../../checks/aria/has-widget-role-evaluate';
import invalidroleEvaluate from '../../checks/aria/invalidrole-evaluate';
import isElementFocusableEvaluate from '../../checks/aria/is-element-focusable-evaluate';
Expand Down Expand Up @@ -187,6 +188,7 @@ const metadataFunctionMap = {
'aria-valid-attr-value-evaluate': ariaValidAttrValueEvaluate,
'fallbackrole-evaluate': fallbackroleEvaluate,
'has-global-aria-attribute-evaluate': hasGlobalAriaAttributeEvaluate,
'has-implicit-chromium-role-matches': hasImplicitChromiumRoleMatches,
'has-widget-role-evaluate': hasWidgetRoleEvaluate,
'invalidrole-evaluate': invalidroleEvaluate,
'is-element-focusable-evaluate': isElementFocusableEvaluate,
Expand Down
7 changes: 7 additions & 0 deletions lib/rules/has-implicit-chromium-role-matches.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
import { getImplicitRole } from '../commons/aria';

function hasImplicitChromiumRoleMatches(node, virtualNode) {
return getImplicitRole(virtualNode, { chromium: true }) !== null;
}

export default hasImplicitChromiumRoleMatches;
7 changes: 7 additions & 0 deletions lib/rules/presentation-role-conflict-matches.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
import { getImplicitRole } from '../commons/aria';

function presentationRoleConflictMatches(node, virtualNode) {
return getImplicitRole(virtualNode, { chromiumRoles: true }) !== null;
}

export default presentationRoleConflictMatches;
1 change: 1 addition & 0 deletions lib/rules/presentation-role-conflict.json
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
{
"id": "presentation-role-conflict",
"matches": "has-implicit-chromium-role-matches",
"selector": "[role=\"none\"], [role=\"presentation\"]",
"tags": ["cat.aria", "best-practice"],
"metadata": {
Expand Down
28 changes: 19 additions & 9 deletions lib/standards/html-elms.js
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,8 @@ const htmlElms = {
},
// Note: if the property applies regardless of variants it is
// placed at the top level instead of the default variant
allowedRoles: ['application']
allowedRoles: ['application'],
chromiumRole: 'Audio'
},
b: {
contentTypes: ['phrasing', 'flow'],
Expand Down Expand Up @@ -143,7 +144,8 @@ const htmlElms = {
},
canvas: {
allowedRoles: true,
contentTypes: ['embedded', 'phrasing', 'flow']
contentTypes: ['embedded', 'phrasing', 'flow'],
chromiumRole: 'Canvas'
},
caption: {
allowedRoles: false
Expand Down Expand Up @@ -205,7 +207,8 @@ const htmlElms = {
},
dl: {
contentTypes: ['flow'],
allowedRoles: ['group', 'list', 'presentation', 'none']
allowedRoles: ['group', 'list', 'presentation', 'none'],
chromiumRole: 'DescriptionList'
},
dt: {
allowedRoles: ['listitem']
Expand All @@ -216,7 +219,8 @@ const htmlElms = {
},
embed: {
contentTypes: ['interactive', 'embedded', 'phrasing', 'flow'],
allowedRoles: ['application', 'document', 'img', 'presentation', 'none']
allowedRoles: ['application', 'document', 'img', 'presentation', 'none'],
chromiumRole: 'EmbeddedObject'
},
fieldset: {
contentTypes: ['flow'],
Expand Down Expand Up @@ -320,7 +324,8 @@ const htmlElms = {
},
iframe: {
contentTypes: ['interactive', 'embedded', 'phrasing', 'flow'],
allowedRoles: ['application', 'document', 'img', 'none', 'presentation']
allowedRoles: ['application', 'document', 'img', 'none', 'presentation'],
chromiumRole: 'Iframe'
},
img: {
variant: {
Expand Down Expand Up @@ -525,7 +530,8 @@ const htmlElms = {
},
label: {
contentTypes: ['interactive', 'phrasing', 'flow'],
allowedRoles: false
allowedRoles: false,
chromiumRole: 'Label'
},
legend: {
allowedRoles: false
Expand Down Expand Up @@ -601,7 +607,8 @@ const htmlElms = {
},
meter: {
contentTypes: ['phrasing', 'flow'],
allowedRoles: false
allowedRoles: false,
chromiumRole: 'progressbar'
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be capitalised? If it isn't in Chromium then not, but it seems inconsistent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, they are all inconsistent in Chrome(ium)

},
nav: {
contentTypes: ['sectioning', 'flow'],
Expand All @@ -623,7 +630,8 @@ const htmlElms = {
contentTypes: ['embedded', 'phrasing', 'flow']
}
},
allowedRoles: ['application', 'document', 'img']
allowedRoles: ['application', 'document', 'img'],
chromiumRole: 'PluginObject'
},
ol: {
contentTypes: ['flow'],
Expand Down Expand Up @@ -814,6 +822,7 @@ const htmlElms = {
svg: {
contentTypes: ['embedded', 'phrasing', 'flow'],
allowedRoles: ['application', 'document', 'img'],
chromiumRole: 'SVGRoot',
namingMethods: ['svgTitleText']
},
sub: {
Expand Down Expand Up @@ -914,7 +923,8 @@ const htmlElms = {
contentTypes: ['embedded', 'phrasing', 'flow']
}
},
allowedRoles: ['application']
allowedRoles: ['application'],
chromiumRole: 'video'
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here. Why is it Audio but not Video?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same answer - it's inconsistently reported in the browser.

},
wbr: {
contentTypes: ['phrasing', 'flow'],
Expand Down
11 changes: 8 additions & 3 deletions test/checks/aria/aria-prohibited-attr.js
Original file line number Diff line number Diff line change
Expand Up @@ -65,19 +65,24 @@ describe('aria-prohibited-attr', function() {
assert.isFalse(checkEvaluate.apply(checkContext, params));
});

it('should allow `elementsAllowedAriaLabel` nodes to have aria-label', function () {
it('should allow `elementsAllowedAriaLabel` nodes to have aria-label', function() {
var params = checkSetup(
'<div id="target" aria-label="hello world"></div>',
{ elementsAllowedAriaLabel: ['div'] }
);
assert.isFalse(checkEvaluate.apply(checkContext, params));
});
it('should not allow `elementsAllowedAriaLabel` nodes with a prohibited role', function () {

it('should not allow `elementsAllowedAriaLabel` nodes with a prohibited role', function() {
var params = checkSetup(
'<div id="target" role="code" aria-label="hello world"></div>',
{ elementsAllowedAriaLabel: ['div'] }
);
assert.isTrue(checkEvaluate.apply(checkContext, params));
});

it('should allow elements that have an implicit role in chromium', function() {
var params = checkSetup('<svg id="target" aria-label="hello world"></svg>');
assert.isFalse(checkEvaluate.apply(checkContext, params));
});
});
17 changes: 16 additions & 1 deletion test/commons/aria/implicit-role.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,20 @@ describe('aria.implicitRole', function() {
assert.isNull(implicitRole(node));
});

it('should return null if there is no implicit role when not considering chromium', function() {
fixture.innerHTML = '<canvas id="target" aria-label="hello"></canvas>';
var node = fixture.querySelector('#target');
flatTreeSetup(fixture);
assert.isNull(implicitRole(node));
});

it('should return the chromium implicit role for elements that have one', function() {
fixture.innerHTML = '<canvas id="target" aria-label="hello"></canvas>';
var node = fixture.querySelector('#target');
flatTreeSetup(fixture);
assert.equal(implicitRole(node, { chromium: true }), 'Canvas');
});

it('should return link for "a[href]"', function() {
fixture.innerHTML = '<a id="target" href>link</a>';
var node = fixture.querySelector('#target');
Expand Down Expand Up @@ -287,7 +301,8 @@ describe('aria.implicitRole', function() {
});

it('should return textbox for "input[type=password][list]"', function() {
fixture.innerHTML = '<input id="target" type="password" list="list"/>' +
fixture.innerHTML =
'<input id="target" type="password" list="list"/>' +
'<datalist id="list"></datalist>';
var node = fixture.querySelector('#target');
flatTreeSetup(fixture);
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
<div id="pass1" role="presentation">Test</div>
<div id="pass2" role="presentation">Test Button</div>
clottman marked this conversation as resolved.
Show resolved Hide resolved
<li id="violation1" role="presentation" aria-label="My Heading">Hello</li>
<h1 id="violation2" role="none" aria-label="My Heading">Hello</h1>
<div id="inapplicable1" role="presentation">Test</div>
<div id="inapplicable2" role="presentation">Test Button</div>
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
{
"rule": "presentation-role-conflict",
"description": "presentation-role-conflict tests",
"violations": [["#violation1"], ["#violation2"]],
"passes": [["#pass1"], ["#pass2"]]
"violations": [["#violation1"], ["#violation2"]]
}
Loading