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 noPresentational option #2549

Merged
merged 1 commit into from
Oct 15, 2020
Merged

feat(get-role): add noPresentational option #2549

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
40 changes: 30 additions & 10 deletions lib/commons/aria/get-role.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,22 +110,15 @@ function hasConflictResolution(vNode) {
}

/**
* Return the semantic role of an element.
*
* @method getRole
* @memberof axe.commons.aria
* @instance
* @method resolveRole
* @param {Element|VirtualNode} node
* @param {Object} options
* @param {boolean} options.noImplicit Do not return the implicit role // @deprecated
* @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
* @see getRole for option details
* @returns {string|null} Role or null
*
* @deprecated noImplicit option is deprecated. Use aria.getExplicitRole instead.
*/
function getRole(node, { noImplicit, ...explicitRoleOptions } = {}) {
function resolveRole(node, { noImplicit, ...explicitRoleOptions } = {}) {
const vNode =
node instanceof AbstractVirtuaNode ? node : getNodeFromTree(node);
if (vNode.props.nodeType !== 1) {
Expand All @@ -152,4 +145,31 @@ function getRole(node, { noImplicit, ...explicitRoleOptions } = {}) {
return explicitRole;
}

/**
* Return the semantic role of an element.
*
* @method getRole
* @memberof axe.commons.aria
* @instance
* @param {Element|VirtualNode} node
* @param {Object} options
* @param {boolean} options.noImplicit Do not return the implicit role // @deprecated
* @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.noPresentational return null if role is presentation or none
* @returns {string|null} Role or null
*
* @deprecated noImplicit option is deprecated. Use aria.getExplicitRole instead.
*/
function getRole(node, { noPresentational, ...options } = {}) {
const role = resolveRole(node, options);

if (noPresentational && ['presentation', 'none'].includes(role)) {
return null;
}
Comment on lines +168 to +170
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this would be better done as an "else if" here https://github.com/dequelabs/axe-core/pull/2549/files#diff-1bf6cc2da8acf89f4e8559d8254862d7L143 instead of in a separate function. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just to be sure I understand the suggestion clearly, I'll explain the reasoning behind my decisions.

I created the resolveRole(...) function from the understanding that the new option needs to handle whatever role gets returned from getRole(...). Since the original getRole(...) function had so many early returns, this was a great way for me to catch "whatever gets returned" as the role, and then apply the new option's behavior.

Let me know if that helps clarify why I did what I did. Thank you @WilcoFiers for your review

Copy link
Contributor

Choose a reason for hiding this comment

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

@Loganras You are absolutely right. @straker reminded me that implicitRole can return presentational too. My suggestion doesn't work. Let's go with this.


return role;
}

export default getRole;
32 changes: 32 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 @@ -380,4 +380,36 @@ describe('aria.getRole', function() {
);
});
});

describe('noPresentational is honored', function() {
it('handles no inheritance role = presentation', function() {
fixture.innerHTML =
'<ul role="presentation" id="target"><li>foo</li></ul>';
flatTreeSetup(fixture);
var node = fixture.querySelector('#target');
assert.isNull(aria.getRole(node, { noPresentational: true }));
});

it('handles inheritance role = presentation', function() {
fixture.innerHTML =
'<ul role="presentation"><li id="target">foo</li></ul>';
flatTreeSetup(fixture);
var node = fixture.querySelector('#target');
assert.isNull(aria.getRole(node, { noPresentational: true }));
});

it('handles implicit role', function() {
var node = document.createElement('img');
node.setAttribute('alt', '');
flatTreeSetup(node);
assert.isNull(aria.getRole(node, { noPresentational: true }));
});

it('handles role = none', function() {
var node = document.createElement('div');
node.setAttribute('role', 'none');
flatTreeSetup(node);
assert.isNull(aria.getRole(node, { noPresentational: true }));
});
});
});