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

feat(rule): aria-allowed-role #945

Merged

jeeyyy merged 38 commits into develop from new-rule-aria-allowed-role

Aug 16, 2018

Contributor

jeeyyy commented Jun 7, 2018 •

edited

Loading

This rule is a taken over PR from the community contribution (PR: #623, @lemnis - thanks for the initial work).

The rule computes if a given role on a node is an allowed aria role.

Closes issue:

New Rule: Disallowed ARIA role for given element #97 (comment)

Reviewer checks

Required fields, to be filled out by PR reviewer(s)

Follows the commit message policy, appropriate for next version
Has documentation updated, a DU ticket, or requires no documentation change
Includes new tests, or was unnecessary
Code is reviewed for security by: @WilcoFiers

PS. @WilcoFiers (please update the above)... I had to do this myself after your approval...

jeeyyy added 2 commits

June 6, 2018 14:16


          feat: initial work for new rule > aria-allowed-role

63c947b


          fix: aria-allowed-role rule

7298f15

jeeyyy changed the title ~~feat: new rule aria-allowed-role~~ feat(rule): aria-allowed-role

This was referenced Jun 7, 2018

New Rule: Disallowed ARIA role for given element #97

Closed

feat(new rule): aria-allowed-role #623

Closed

This comment has been minimized.

Sign in to view

jeeyyy added 3 commits

June 7, 2018 09:04


          chore: merge from > develop

ee23a9d


          fix: integration test fixes.

03af3af


          chore: merge from > develop

9646b5b

jeeyyy requested review from marcysutton, WilcoFiers, stephenmathieson, akornmeier and isner

June 7, 2018 11:19

jeeyyy self-assigned this

jeeyyy added the rules label

stephenmathieson reviewed

View reviewed changes

lib/checks/aria/aria-allowed-role.js Outdated

+              		}
+              		if (node.href) {
+              			return axe.commons.aria.isAllowedSubsetRole('aWithHref', role);
+              		} else {

This comment was marked as resolved.

Sign in to view

stephenmathieson reviewed

View reviewed changes

lib/checks/aria/aria-allowed-role.js Outdated

+              		if (node.alt) {
+              			// any role except the roles used by empty alt values
+              			return !hasAllowedEmptyAltRole;
+              		} else {

This comment was marked as resolved.

Sign in to view

stephenmathieson reviewed

View reviewed changes

lib/checks/aria/aria-allowed-role.js Outdated

+              		case 'button':
+              			return axe.commons.aria.isAllowedSubsetRole('inputTypeButton', role);
+              		case 'checkbox':
+              			if (role === 'button' && node.hasAttribute('aria-pressed')) {

This comment was marked as resolved.

Sign in to view

This comment was marked as resolved.

Sign in to view

stephenmathieson reviewed

View reviewed changes

lib/checks/aria/aria-allowed-role.js Outdated

+              		const hasImplicitListitemRole = node.matches('ol li, ul li');
+              		if (hasImplicitListitemRole) {
+              			return axe.commons.aria.isAllowedSubsetRole('li', role);
+              		} else {

This comment was marked as resolved.

Sign in to view

stephenmathieson reviewed

View reviewed changes

lib/commons/aria/roles.js Outdated

@@ @@ -123,9 +122,8 @@ aria.implicitNodes = function (role) { @@
                * @memberof axe.commons.aria
                * @instance
                * @param  {HTMLElement} node The node to test
-               * @return {Mixed}      Either the role or `null` if there is none
+               * @return {Mixed}      			Either the role or `null` if there is none

This comment was marked as outdated.

Sign in to view

This comment was marked as resolved.

Sign in to view

stephenmathieson reviewed

View reviewed changes

lib/commons/aria/roles.js Outdated

+               * @memberof axe.commons.aria
+               * @instance
+               * @param 	{String} 	tagName 	tag name of a node
+               * @param 	{String} 	role 			aria role to check

This comment was marked as resolved.

Sign in to view

This comment has been minimized.

Sign in to view

isner reviewed

View reviewed changes

lib/checks/aria/aria-allowed-role.js Outdated

+              const implicitRole = axe.commons.aria.implicitRole(node);
+              /**
+               * Helper object with functions to computed is role allowed for a given tag

This comment was marked as resolved.

Sign in to view


          refactor: amends based on review.

5a040ef

This comment has been minimized.

Sign in to view

jeeyyy requested a review from dylanb

June 8, 2018 17:22

dylanb previously requested changes

View reviewed changes

Contributor

dylanb left a comment

This is a bad API design for the functions aria.isAllowedRole and aria.isAllowedSubsetRole and the fact that much of the logic for which role is allowed is buried inside the check and not in the utility function.

I would like to see that logic taken out of the check and put into the utility function so that the utility function can be re-used atomically. I.e. the only public API should be aria.isRoleAllowed and this function should embed all the logic to know when to call things like aria.isAllowedSubsetRole - which should be renamed so its clear its an internal function.

lib/commons/aria/index.js Outdated

@@ @@ -13,19 +13,33 @@ lookupTable.attributes = { @@
               	},
               	'aria-atomic': {
               		type: 'boolean',
-              		values: ['true', 'false']
+              		values: [

This comment was marked as resolved.

Sign in to view

lib/commons/aria/roles.js Outdated

@@ @@ -5,7 +5,7 @@ @@
                * @method isValidRole
                * @memberof axe.commons.aria
                * @instance
-               * @param  {String}  role The role to check
+               * @param {String} role The role to check

This comment was marked as resolved.

Sign in to view

jeeyyy changed the title ~~feat(rule): aria-allowed-role~~ [WIP] feat(rule): aria-allowed-role


          fix: merge from develop

jeeyyy added 2 commits

August 6, 2018 14:08


          fix: updates based on review

02c15cf


          Merge branch 'develop' into new-rule-aria-allowed-role

jeeyyy dismissed WilcoFiers’s stale review

August 6, 2018 13:15

PR Updated. Review again.

WilcoFiers reviewed

View reviewed changes

lib/commons/aria/get-element-unallowed-roles.js Outdated

+              	const roleSegments = getRoleSegments(node);
+              	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
+              	return roleSegments.filter(role => {

Contributor

WilcoFiers Aug 7, 2018

Why aren't you returning booleans in this?

Contributor Author

jeeyyy Aug 7, 2018

We have discussed this in the past, the result of this filter is an array of unallowed roles for a given element.

This array is returned to aria-allowed-role check, which needs to populate the data for doT templating with an listing of roles for fail message.

Hence we need to return role, than a boolean.

Contributor

WilcoFiers Aug 7, 2018

Sorry Jey. I meant inside the callback. You're doing return; and return role; instead of return false / return true. It's nitpicky, I know. But returning undefined rather than false, and returning a string rather than true makes it look like this code is supposed to do something that it doesn't actually do.

WilcoFiers reviewed

View reviewed changes

lib/commons/aria/get-element-unallowed-roles.js Outdated

+              	// by pass other elements that may not be a known html element
+              	if (
+              		document.createElement(tagName).toString() === '[object HTMLUnknownElement]'

Contributor

WilcoFiers Aug 7, 2018

Unnecessary due to line 47. Please remove.

Contributor Author

jeeyyy Aug 7, 2018

Custom components that are written to override generic html tags still do not change the type of tag. Tested here - https://jsfiddle.net/Jey/bprv8fqe/7/

Hence removing this check.

WilcoFiers previously requested changes

View reviewed changes

test/checks/aria/aria-allowed-role.js Outdated

+              	it('should pass when A has namespace as svg', function() {
+              		var node = document.createElement('a');
+              		node.setAttribute('xmlns', 'http://www.w3.org/2000/svg');

Contributor

WilcoFiers Aug 7, 2018

You can't set the namespace of an element after its created. This test doesn't work. You need to do document.createElementNS('http://www.w3.org/2000/svg', 'a') instead.

lib/commons/aria/get-element-unallowed-roles.js Outdated

+              	const roleSegments = getRoleSegments(node);
+              	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
+              	return roleSegments.filter(role => {

Contributor

WilcoFiers Aug 7, 2018

Sorry Jey. I meant inside the callback. You're doing return; and return role; instead of return false / return true. It's nitpicky, I know. But returning undefined rather than false, and returning a string rather than true makes it look like this code is supposed to do something that it doesn't actually do.

Contributor

dylanb commented Aug 7, 2018

Does it make sense to start this pull request over with a single commit?


          Merge branch 'develop' into new-rule-aria-allowed-role

d69a797

Contributor Author

jeeyyy commented Aug 7, 2018

@dylanb - Probably worth taking into a new PR, or squash and merge. I will speak to @WilcoFiers to make a decision. Thanks.

jeeyyy added 2 commits

August 8, 2018 10:06


          fix: improve test and refactor based on review

87929c7


          Merge branch 'develop' into new-rule-aria-allowed-role

7a0df2c

jeeyyy dismissed WilcoFiers’s stale review

August 8, 2018 09:07

PR Updated. Review again.

Member

stephenmathieson commented Aug 8, 2018

Does it make sense to start this pull request over with a single commit?

Since this PR only adds a single feature, I think it makes much more sense to just squash it rather than doing any "extra" work.

jeeyyy changed the title ~~feat(rule): aria-allowed-role~~ [WIP] feat(rule): aria-allowed-role

Contributor

dylanb commented Aug 10, 2018

I agree with @stephenmathieson : squash

jeeyyy added 2 commits

August 11, 2018 14:04


          fix: enhance no role and any role as per aria spec

2d86762


          test: add tests for aria-allowed-role related functions

8f16a86

jeeyyy changed the title ~~[WIP] feat(rule): aria-allowed-role~~ feat(rule): aria-allowed-role


          Merge branch 'develop' into new-rule-aria-allowed-role

d72a247

Contributor Author

jeeyyy commented Aug 12, 2018

@dylanb - yup, will be squashing it. Cheers.

Contributor Author

jeeyyy commented Aug 12, 2018

@WilcoFiers - Please review again. Have made changes based on our previous chat. Have enhanced no role and any role as per https://www.w3.org/TR/html-aria/ and enhanced tests.

@dylanb @marcysutton - reviews welcome


          test: fix tests allowedElement object structure

ac0ba14

WilcoFiers reviewed

View reviewed changes

lib/commons/aria/validate-node-and-attributes.js

+               * @param {Array<String|Object>} constraintsArray an array containing TAGNAME or an OBJECT abstraction with conditions and attributes
+               * @return {Boolean} true/ false based on if node passes the constraints expected
+               */
+              aria.validateNodeAndAttributes = function validateNodeAndAttributes(

Contributor

WilcoFiers Aug 15, 2018

I think this one would probably be a little easier if instead of passing in an array of constraints, you had passed it one constraint. It simplifies the function quite a bit and it makes it more generic. I think something like a "nodeMatchesDefinition" where we can pass it different things could be useful in other places as well.

I don't think this needs to change now, but it's something to think about.

jeeyyy mentioned this pull request

feat: flag unsupported roles #1064

Merged

4 tasks


          chore: merge from develop

2777dbc

jeeyyy requested a review from a team as a code owner

August 16, 2018 13:39

Contributor Author

jeeyyy commented Aug 16, 2018

Conflicts resolve. Review/ Approve.

WilcoFiers approved these changes

View reviewed changes

lib/commons/aria/get-element-unallowed-roles.js

+              	const tagName = node.nodeName.toUpperCase();
+              	// by pass custom elements
+              	if (!axe.utils.isHtmlElement(node)) {

Contributor

WilcoFiers Aug 16, 2018

I wonder if we shouldn't move this into the matcher instead; make these nodes inapplicable instead of passing them. Seems slightly more appropriate. Maybe just create a ticket for it?

lib/rules/aria-allowed-role.json

+              {
+              	"id": "aria-allowed-role",
+              	"excludeHidden": false,
+              	"selector": "[role]",

Contributor

WilcoFiers Aug 16, 2018

We haven't disabled the dpub rule yet. Include or open a new issue for it.

test/commons/aria/validate-node-and-attributes.js

+              		it('ensure that the lookupTable.elementsAllowedNoRole is invoked', function() {
+              			var overrideInvoked = false;
+              			axe.commons.aria.lookupTable.elementsAllowedNoRole = [

Contributor

WilcoFiers Aug 16, 2018

Why are you overriding the lookup table at all here? You could just pass in this array, no?

jeeyyy merged commit c270a46 into develop

jeeyyy deleted the new-rule-aria-allowed-role branch

August 16, 2018 15:33

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

isner isner left review comments

marcysutton marcysutton left review comments

stephenmathieson stephenmathieson left review comments

rdeltour rdeltour requested changes

WilcoFiers WilcoFiers approved these changes

dylanb dylanb left review comments

akornmeier Awaiting requested review from akornmeier

Labels

rules