feat: Add aria.getRole method #1017

WilcoFiers · 2018-07-20T18:39:59Z

I've got some uses lined up for this. This function is over due :), and is going to help us deal with multiple roles properly at some point in the future.

This PR is awaiting merger of #1015.

Closes issue: none

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: Jey (@JKODU)

jeeyyy · 2018-07-28T20:05:52Z

doc/rule-descriptions.md

-| landmark-no-duplicate-banner | Ensures the document has no more than one banner landmark | Moderate | cat.semantics, best-practice | true |
-| landmark-no-duplicate-contentinfo | Ensures the document has no more than one contentinfo landmark | Moderate | cat.semantics, best-practice | true |
-| landmark-one-main | Ensures a navigation point to the primary content of the page. If the page contains iframes, each iframe should contain either no main landmarks or just one | Moderate | cat.semantics, best-practice | true |
+| landmark-banner-is-top-level | Ensures the banner landmark is at top level | Moderate | cat.semantics, best-practice | true |


Minor remark, shouldn't these documentation updates be a part of another build where these changes were made to the respective rules? Worth folding into a new PR?

jeeyyy · 2018-07-28T20:09:33Z

lib/commons/aria/get-role.js

+	});
+	const role = validRoles[0];
+
+	// Get the impgit licit role, if permitted


Edit: In comment - implicit

jeeyyy · 2018-07-28T20:17:08Z

lib/commons/aria/get-role.js

+		}
+		return aria.isValidRole(role, { allowAbstract: abstracts });
+	});
+	const role = validRoles[0];


need a length check on validRoles here, there is a chance that this can be an empty array, if role is not defined in a given node.

Doesn't matter. In that case role undefined, which is what it should be.

jeeyyy · 2018-07-28T20:36:48Z

@WilcoFiers
I also think this could have been an enhancement to existing getRoleSegments method with an optional argument to return just the first valid role.

WilcoFiers

@JKODU getRoleSegments doesn't exist yet. It is new in #945. Given Romain's comment about support for type, I don't think we need it. Most of what it does is covered by axe.utils.tokenList.

WilcoFiers · 2018-08-05T17:14:01Z

lib/commons/aria/get-role.js

+		}
+		return aria.isValidRole(role, { allowAbstract: abstracts });
+	});
+	const role = validRoles[0];


Doesn't matter. In that case role undefined, which is what it should be.

WilcoFiers · 2018-08-05T17:22:00Z

@marcysutton @JKODU Can you please review. I've got a separate fix I want to do that relies on this getting in.

jeeyyy · 2018-08-05T18:02:59Z

lib/commons/aria/get-role.js

+	});
+	const explicitRole = validRoles[0];
+
+	// Get the implicit licit role, if permitted


editorial: implicit licit...

jeeyyy · 2018-08-05T18:05:12Z

lib/commons/aria/get-role.js

@@ -0,0 +1,25 @@
+/* global aria, axe */
+


Add jsdoc comments please, with @return decorating what is expected.

jeeyyy · 2018-08-05T18:05:55Z

lib/commons/aria/get-role.js

+		}
+		return aria.isValidRole(role, { allowAbstract: abstracts });
+	});
+	const explicitRole = validRoles[0];


So if validRoles is an empty array, explicitRole will be undefined.
Line 24 below will do a undefined || null and null will be returned. Is that what is expected? Asking, as in some places we expect an undefined, so long as it is document and we agree on the return, happy with this.

Right, tests below, tell me null is expected as return. So all good.

jeeyyy

LGTM, asides jsdoc comments.

marcysutton · 2018-08-06T22:58:39Z

test/commons/aria/get-role.js

+		});
+
+		it('still returns the explicit role', function() {
+			var node = document.createElement('li', { noImplicit: true });


Is this noImplicit option supposed to go in the call to aria.getRole instead?

Yikes! Indeed it is.

…a-getrole

WilcoFiers · 2018-08-07T09:41:25Z

Updated, please look again.

Review 10% (50 max) of the following (Product manager only): - [x] feat: Update FR (french) translation file for 3.1 release (#1089) (4a5cad0) - [x] chore: Fix example dependency mistake (#1094) (6af5733) - [x] fix: Do not require media captions / descriptions (#1075) (289f623) - [x] feat(aria-role): Remove non-existing "text" role (#1069) (67ec1f5) - [x] Merge pull request #1017 from dequelabs/aria-getrole (f64ff10) - [x] chore: remove `.jshintrc` files (#1028) (03b3327) - [x] chore: ability to add external libs (axios) (0957dab) - [x] Merge pull request #985 from dequelabs/git-update (54b0b60)

WilcoFiers added 2 commits July 20, 2018 20:06

fix: Ignore abstracts in determining element roles

e3b1e1d

feat: Add aria.getRole method

1d2a0e4

jeeyyy reviewed Jul 28, 2018

View reviewed changes

WilcoFiers and others added 3 commits August 2, 2018 02:35

Merge branch 'develop' into aria-getrole

2aab267

chore(get-role): Minor readability fix

74aaa0e

Merge branch 'develop' into aria-getrole

fb2aa46

WilcoFiers requested review from dylanb and marcysutton August 5, 2018 17:16

WilcoFiers commented Aug 5, 2018

View reviewed changes

WilcoFiers changed the title ~~(WIP) feat: Add aria.getRole method~~ feat: Add aria.getRole method Aug 5, 2018

jeeyyy reviewed Aug 5, 2018

View reviewed changes

jeeyyy previously approved these changes Aug 5, 2018

View reviewed changes

marcysutton reviewed Aug 6, 2018

View reviewed changes

WilcoFiers and others added 3 commits August 7, 2018 11:27

chore(getRole): Renamed opt.tokenList to opt.fallback

514da4d

Merge branch 'develop' into aria-getrole

fcb9f8c

Merge branch 'aria-getrole' of github.com:dequelabs/axe-core into ari…

786a62d

…a-getrole

WilcoFiers dismissed jeeyyy’s stale review via 786a62d August 7, 2018 09:32

chore: Fix oops

e79a039

dylanb approved these changes Aug 7, 2018

View reviewed changes

Merge branch 'develop' into aria-getrole

56ceee9

WilcoFiers merged commit f64ff10 into develop Aug 7, 2018

WilcoFiers deleted the aria-getrole branch August 7, 2018 22:41

WilcoFiers mentioned this pull request Aug 27, 2018

Release v3.1.0 #1102

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add aria.getRole method #1017

feat: Add aria.getRole method #1017

WilcoFiers commented Jul 20, 2018 •

edited by jeeyyy

Loading

jeeyyy Jul 28, 2018

jeeyyy Jul 28, 2018

jeeyyy Jul 28, 2018 •

edited

Loading

WilcoFiers Aug 5, 2018

jeeyyy commented Jul 28, 2018

WilcoFiers left a comment •

edited

Loading

WilcoFiers Aug 5, 2018

WilcoFiers commented Aug 5, 2018

jeeyyy Aug 5, 2018

jeeyyy Aug 5, 2018

jeeyyy Aug 5, 2018

jeeyyy Aug 5, 2018

jeeyyy left a comment

marcysutton Aug 6, 2018

WilcoFiers Aug 7, 2018

WilcoFiers commented Aug 7, 2018

feat: Add aria.getRole method #1017

feat: Add aria.getRole method #1017

Conversation

WilcoFiers commented Jul 20, 2018 • edited by jeeyyy Loading

Reviewer checks

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeeyyy Jul 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeeyyy commented Jul 28, 2018

WilcoFiers left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WilcoFiers commented Aug 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeeyyy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WilcoFiers commented Aug 7, 2018

WilcoFiers commented Jul 20, 2018 •

edited by jeeyyy

Loading

jeeyyy Jul 28, 2018 •

edited

Loading

WilcoFiers left a comment •

edited

Loading