Support options in aria-allowed-attr and aria-required-attr #673

marcysutton · 2018-01-05T00:54:28Z

This continues the work to avoid having to manipulate the lookupTable directly. valid-attr and valid-attr-value already used options; this PR adds options to aria-allowed-attr and aria-required-attr. The usage for these checks is now to plumb in an options object with a role and an array of accepted attribute values:

axe.configure({
    checks: [{
        id: 'aria-allowed-attr',
        options: {
            separator: ['aria-valuenow', 'aria-valuemin', 'aria-valuemax']
        }
    }]
});
axe.run(document,  {}, function(error, results) {
    assert.equal(results.violations.length, 0);
    done();
});

Closes #644

cc @waabid @iamrafan

WilcoFiers · 2018-01-05T11:43:28Z

lib/checks/aria/allowed-attr.js

@@ -12,7 +14,7 @@ if (role && allowed) {
 	for (var i = 0, l = attrs.length; i < l; i++) {
 		attr = attrs[i];
 		attrName = attr.name;
-		if (axe.commons.aria.validateAttr(attrName) && allowed.indexOf(attrName) === -1) {
+		if (options.indexOf(attrName) === -1 && axe.commons.aria.validateAttr(attrName) && allowed.indexOf(attrName) === -1) {


You should wrap this line. You may also want to use .includes.

marcysutton · 2018-01-06T00:40:23Z

I incorporated the small piece of feedback and also added options to required-attr, as well as some integration tests for axe.configure. There are other checks that (indirectly) reference the lookupTable, but they're quite complex (like required-children) so unless there's a specific need for those, I'll keep the scope of this PR small.

Edit: also, I thought about documenting options for these checks but I wasn't sure where that information should live. If anyone has suggestions let me know :)

WilcoFiers

Looks good. The one thing I'm hesitant about with this is that this is starting to look a lot like accessibility supported functionality. We haven't got an API design for this yet, and I'm hoping we won't have to break this once settle on a way to do this.

WilcoFiers · 2018-01-06T12:42:56Z

test/checks/aria/allowed-attr.js

@@ -119,4 +119,11 @@ describe('aria-allowed-attr', function () {
 		assert.isNull(checkContext._data);
 	});

+	describe('options', function () {
+		it('should allow provided attribute names', function () {
+			fixture.innerHTML = '<div role="separator" id="target" aria-valuenow="0" aria-valuemin="-2" aria-valuemax="4"></div>';


You may want to use fake aria properties instead. Otherwise, once we've added those values, this test becomes invalid.

WilcoFiers · 2018-01-06T12:44:36Z

test/checks/aria/required-attr.js

@@ -57,4 +57,12 @@ describe('aria-required-attr', function () {
 		axe.commons.aria.requiredAttr = orig;
 	});

+	describe('options', function () {
+		it('should require provided attribute names', function () {
+			fixture.innerHTML = '<div role="slider" id="target"></div>';


Same here, you should use a made up role. May I suggest role="McCheddarton"? :)

Love it! will do.

WilcoFiers · 2018-01-06T12:55:23Z

lib/checks/aria/allowed-attr.js

@@ -1,3 +1,5 @@
+options = Array.isArray(options) ? options : [];


I was wondering if this should allow per role specification, instead of (or in addition to) a generic "allowed everywhere". So you could do: { separator: ['aria-valuenow', 'aria-valuemin', 'aria-valuemax'] }. You could still allow the array, and you could add a wildcard option for the "generic" case you've got now: { '*': ['always-allowed'] }.

WilcoFiers · 2018-01-06T12:56:31Z

lib/checks/aria/required-attr.js

@@ -1,10 +1,21 @@
+options = Array.isArray(options) ? options : [];


Same as my comment above. This is a generic "required everywhere". That seems a bit useless. You wouldn't require a particular attribute on every element. You'd need to set certain things to be required for certain roles.

Sounds good, thanks for the tip!

marcysutton · 2018-01-10T06:44:20Z

The one thing I'm hesitant about with this is that this is starting to look a lot like accessibility supported functionality. We haven't got an API design for this yet, and I'm hoping we won't have to break this once settle on a way to do this.

We can make changes when we're ready, but these options will help support our users now. Options are already used in the codebase in a few places so there's precedent. But perhaps we can hold off from a big documentation push until we decide on an API.

e.g. {separator: ['aria-valuenow', 'aria-valuemin', aria-valuemax']}

marcysutton · 2018-01-10T20:46:41Z

I've updated the PR so the checks now take an object as options:

{ 'separator': ['aria-valuemin', 'aria-valuemax', 'aria-valuenow']}

WilcoFiers · 2018-01-11T10:21:23Z

lib/checks/aria/allowed-attr.js

 allowed = axe.commons.aria.allowedAttr(role);
+
+if (Object.keys(options).length) {


You can simplify this.

if (Array.isArray(options[role])) { allowed = axe.utils.uniqueArray(options[role].concat(allowed)); }

Done. I also removed a duplicate to-array file in axe.commons. It was almost exactly the same code duplicated, we don't want to maintain both if we don't have to.

Oops, I was confused by this comment...I've made the change and added it to this PR. Thanks!

The exact same code existed in axe.utils and axe.commons (as axe.utils.toArray)

* chore: rename lut in tests * feat: allow options in aria-allowed-attr * chore: rename options integ. tests for clarity * feat: add required-attr options, integration tests * chore: PR feedback for aria-allowed-attr * feat: use object for ARIA check options e.g. {separator: ['aria-valuenow', 'aria-valuemin', aria-valuemax']} * chore: remove duplicate to-array util The exact same code existed in axe.utils and axe.commons (as axe.utils.toArray) * chore: simplify ARIA options usage

isner · 2018-01-18T14:55:46Z

test/integration/full/configure-options/configure-options.js

+	describe('aria-allowed-attr', function() {
+		it('should allow an attribute supplied in options', function(done) {
+			target.setAttribute('role', 'separator');
+			target.setAttribute('aria-valuenow', '0');


In the event that 'aria-valuenow' becomes an allowed attribute in our lookup table, this test won't be testing anything anymore. Just to make the test more future-proof, I suggest using a random value like 'peanutbutter' instead of a real attribute so we can guarantee the test will always be testing the override functionality with a truly unexpected value.

isner · 2018-01-18T15:04:19Z

lib/core/utils/to-array.js

+ * @return {Array}
+ */
+axe.utils.uniqueArray = (arr1, arr2) => {


This is super minor, but it'd be nice if this iterated over an arbitrary number of array inputs.

…#673) * chore(ci): update selenium-webdriver to latest * fix tests * udpate webdriverjs

chore: rename lut in tests

66646a8

marcysutton added the support label Jan 5, 2018

marcysutton requested review from WilcoFiers and isner January 5, 2018 00:54

WilcoFiers reviewed Jan 5, 2018

View reviewed changes

WilcoFiers approved these changes Jan 5, 2018

View reviewed changes

Marcy Sutton added 3 commits January 5, 2018 16:18

feat: allow options in aria-allowed-attr

cf9af49

chore: rename options integ. tests for clarity

e30a372

feat: add required-attr options, integration tests

4f266c7

marcysutton force-pushed the support-rule-options branch from f4d8011 to 4f266c7 Compare January 6, 2018 00:27

marcysutton changed the title ~~Support options in aria-allowed-attr~~ Support options in aria-allowed-attr and aria-required-attr Jan 6, 2018

chore: PR feedback for aria-allowed-attr

69fba9d

WilcoFiers reviewed Jan 6, 2018

View reviewed changes

feat: use object for ARIA check options

fd6d671

e.g. {separator: ['aria-valuenow', 'aria-valuemin', aria-valuemax']}

WilcoFiers reviewed Jan 11, 2018

View reviewed changes

Marcy Sutton added 2 commits January 18, 2018 10:07

chore: remove duplicate to-array util

86fd5bf

The exact same code existed in axe.utils and axe.commons (as axe.utils.toArray)

chore: simplify ARIA options usage

a51514d

marcysutton force-pushed the support-rule-options branch from 58c9442 to a51514d Compare January 18, 2018 12:37

WilcoFiers approved these changes Jan 18, 2018

View reviewed changes

marcysutton merged commit ffae2c0 into develop-2x Jan 18, 2018

marcysutton mentioned this pull request Jan 18, 2018

Ensure rule/check options can be configured to override default selectors #644

Closed

isner reviewed Jan 18, 2018

View reviewed changes

dylanb deleted the support-rule-options branch March 8, 2018 19:01

mrtnvh pushed a commit to mrtnvh/axe-core that referenced this pull request Nov 24, 2023

chore(ci,webdriverjs): update selenium-webdriver to latest (dequelabs…

b142be9

…#673) * chore(ci): update selenium-webdriver to latest * fix tests * udpate webdriverjs

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support options in aria-allowed-attr and aria-required-attr #673

Support options in aria-allowed-attr and aria-required-attr #673

marcysutton commented Jan 5, 2018 •

edited

Loading

WilcoFiers Jan 5, 2018

marcysutton commented Jan 6, 2018 •

edited

Loading

WilcoFiers left a comment

WilcoFiers Jan 6, 2018

WilcoFiers Jan 6, 2018

marcysutton Jan 10, 2018

WilcoFiers Jan 6, 2018

WilcoFiers Jan 6, 2018

marcysutton Jan 10, 2018

marcysutton commented Jan 10, 2018

marcysutton commented Jan 10, 2018

WilcoFiers Jan 11, 2018

marcysutton Jan 18, 2018

marcysutton Jan 18, 2018

isner Jan 18, 2018

isner Jan 18, 2018

		@@ -1,3 +1,5 @@
		options = Array.isArray(options) ? options : [];

		@@ -1,10 +1,21 @@
		options = Array.isArray(options) ? options : [];

		allowed = axe.commons.aria.allowedAttr(role);

		if (Object.keys(options).length) {

Support options in aria-allowed-attr and aria-required-attr #673

Support options in aria-allowed-attr and aria-required-attr #673

Conversation

marcysutton commented Jan 5, 2018 • edited Loading

Choose a reason for hiding this comment

marcysutton commented Jan 6, 2018 • edited Loading

WilcoFiers left a comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcysutton commented Jan 10, 2018

marcysutton commented Jan 10, 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

Choose a reason for hiding this comment

marcysutton commented Jan 5, 2018 •

edited

Loading

marcysutton commented Jan 6, 2018 •

edited

Loading