feat(commons): Add matches methods #1270

WilcoFiers · 2018-12-09T17:49:47Z

This PR adds a series of "matches" functions to axe.commons.matches. These functions are designed to allow us to match elements on different types of things. We started doing this for the aria-allowed-role rule, but while working on the accessible name computation I found myself building a very similar thing.

This PR replaces the existing aria.validateNodeAndAttributes with a more general purpose axe.commons.matches function.

Note, I realise there is a bunch more refactoring that needs to happen. I'd prefer to keep that out of this PR, so that we can get the accessible name PR resolved.

Note This PR removes a commons that was introduced in 3.1.0. This is a breaking change. I do not think this will impact anyone, and so I didn't put in the time to put some deprecation thing around it, but if anyone thinks this is necessary I can put that in.

This PR is a prerequisite for #1163

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: @JKODU

jeeyyy

Some comments, see inline.

Yet to review the tests.

I have also noticed that prettier has not run, for these commits. May be worth updating the branch, to be inline with develop.

jeeyyy · 2018-12-10T14:40:00Z

lib/commons/aria/index.js

+function isNull (value) {
+	return value === null
+}
+function notNull (value) {


isNotNull would be better naming, to go inline with isNull

jeeyyy · 2018-12-10T14:41:51Z

lib/commons/aria/index.js

-			elementConditions.MUST_HAVE_SIZE_ATTRIBUTE_WITH_VALUE_GREATER_THAN_1,
-		attributes: {
-			TYPE: 'MULTIPLE'
+		nodeName: 'select',


💯 for all the nodeName changes as against tagName. Nice. 🎆

jeeyyy · 2018-12-10T14:44:25Z

lib/commons/matches/attributes.js

@@ -0,0 +1,8 @@
+/* global matches */
+matches.attributes = function matchesAttributes (node, matcher) {


jsdoc comments.

Where/how does this method get called?

You can call it directly, or use it as a property on matches.fromDefinition() or .matches(). I'll add a comment into matches.fromDefinition like you suggested.

jeeyyy · 2018-12-10T14:48:38Z

lib/commons/matches/from-function.js

+
+	// Check that the property has all the expected values
+	return Object.keys(matcher).every(propName => {
+		return matches.fromString(getValue(propName), matcher[propName]);


As per this line - https://github.com/dequelabs/axe-core/pull/1270/files#diff-751a09d50f40e024172d208c3beb465bR5

the getValue(propName) sometimes can return an undefined if attribute is not defined.

It would be good to add some preventive code either in the function as an argument which is passed in as getValue, or in the fromString fn.

Hmm... array.includes(undefined) could be a problem I suppose. Let me think about it...

Was this addressed @WilcoFiers?

Yes, here:
https://github.com/dequelabs/axe-core/pull/1270/files#diff-3bbb4ce58de474cbc3dc418d3bf0de72R25

That's the only scenario where I expect difficulties.

jeeyyy · 2018-12-10T14:49:00Z

lib/commons/matches/from-definition.js

+  'condition'
+]
+
+matches.fromDefinition = function matchFromDefinition (node, definition) {


jsdoc comments.

jeeyyy · 2018-12-10T14:49:47Z

lib/commons/matches/from-definition.js

+  return Object.keys(definition)
+  .every(matcherName => {
+    if (!matchers.includes(matcherName)) {
+      console.log(definition)


should we surround this by the flag we have for logging in axe? debug: true?

This doesn't belong here it all. I should remove it. Good catch.

jeeyyy · 2018-12-10T14:51:30Z

lib/commons/matches/from-definition.js

+  })
+}
+
+matches.condition = function (node, condition) {


Where is this used?

Also, perhaps worth having this in a separate file if there is a need for it.

There are several "condition" things in the aria/index.js file. I didn't put it in its own file because I'm lazy, and it's a single line function. Didn't seem worth the effort. Please let me know if you think this needs to be done.

Just for separation of concerns, I would put this in a single file.

It definitely needs some comments about usage. Similar to matches.attributes, it's difficult to follow how the lookupTable functions with this matches object...

jeeyyy · 2018-12-10T14:52:15Z

lib/commons/matches/from-function.js

@@ -0,0 +1,15 @@
+/* global matches */
+matches.fromFunction = function matchFromFunction(getValue, matcher) {


jsdoc comments

jeeyyy · 2018-12-10T14:59:38Z

lib/commons/matches/node-name.js

+	if (typeof isXHTMLGlobal === 'undefined') {
+		isXHTMLGlobal = axe.utils.isXHTML(node.ownerDocument);
+	}
+	if (typeof isXHTML === 'undefined') {


The order of these two if's can be changed, so the need to follow a code path that executes probably only once is kept out of view.

There is no need to instantiate isXHTMLGlobal, if isXHTML is passed every time as an argument.

let isXHTMLGlobal; matches.nodeName = function matchNodeName (node, matcher, { isXHTML = undefined } = {}) { node = node.actualNode || node; if (typeof isXHTML === 'undefined') { if (typeof isXHTMLGlobal === 'undefined') { isXHTMLGlobal = axe.utils.isXHTML(node.ownerDocument); } isXHTML = isXHTMLGlobal } ... }

Was this addressed @WilcoFiers?

Yes. I added in Dylan's axe.utils.matchesSelector suggestion as well, but this follows the structure you suggested.

jeeyyy · 2018-12-10T15:02:24Z

lib/commons/matches/properties.js

+matches.properties = function matchesProperties (node, matcher) {
+  node = node.actualNode || node;
+  const out = matches.fromFunction(
+    propName => node[propName],


do we need an undefined handler here?

No. fromString will handle this.

dylanb

Left some comments and ran out of time...

dylanb · 2018-12-11T20:33:18Z

lib/commons/matches/index.js

@@ -0,0 +1,51 @@
+/* exported matches */


Do we need to document this function for consumers of axe-core?

At some point maybe. I've been thinking about potentially using them as rule selectors. But while they are commons? We didn't document our other commons, except through JSDocs (which I'll do), so I don't think that needs to be in this PR. Happy to raise an issue for it though so we know to do it later.

We have introduced a npm script npm run api-docs, which generates jsdoc's based API documentation. This can be accessed via http://localhost/doc/api.

See - https://github.com/dequelabs/axe-core/blob/develop/doc/API.md#full-api-reference-for-developers

IMO commons functions should all be documented, especially with the increasing team collaboration coming up.

Like Jey said, collaborators can generate docs for commons.

dylanb · 2018-12-11T20:44:42Z

test/commons/matches/from-definition.js

+      'bar',
+      ['bar', 'baz'],
+      /bar/,
+      function (nodeName) {


should this function be changed to

function (attributeValue) { return attributeValue === 'bar' }

In order to make the use case more clear?

dylanb · 2018-12-11T20:48:21Z

test/commons/matches/from-definition.js

+    }))
+  })
+
+  it('works with virtual nodes', function () {


should we test more conditions against virtual nodes? E.g. property matchers?

dylanb · 2018-12-11T20:49:26Z

test/commons/matches/from-definition.js

+  describe('with a `condition` property', function () {
+    it('calls condition and uses its return value as a matcher', function () {
+      fixture.innerHTML = '<div>foo</div>'
+      assert.isTrue(fromDefinition(fixture.firstChild, {


Should we pass the virtualNode into here or the node? Are there scenarios where the virtualNode would be needed?

I don't think there are. Certainly not with the matchers that are in here now.

dylanb · 2018-12-11T20:57:56Z

lib/commons/matches/from-definition.js

+	if (Array.isArray(definition)) {
+		return definition.some(definition => matches(node, definition));
+  }
+	if (typeof definition === 'string') {


Why not use matchesSelector for all things that can be turned into a CSS selector (e.g. attributes)?

Only attributes with strings can be turned into selectors. This seemed like a cleaner way to do it. If you don't mind, I'd prefer to get this in and open an issue to investigate that separately.

I've opened issue #1279 to look into this.

dylanb · 2018-12-11T21:00:10Z

lib/commons/matches/node-name.js

+/* global matches */
+let isXHTMLGlobal
+matches.nodeName = function matchNodeName (node, matcher, { isXHTML } = {}) {
+	node = node.actualNode || node;


Why not use matchesSelector here?

That works only if the matcher is a string, but sure. I'll do that.

Please update your review

jeeyyy

Some comments. Also, please re-run prettier and commit again.

jeeyyy · 2018-12-17T15:34:33Z

lib/commons/matches/from-definition.js

+  })
+}
+
+matches.condition = function (node, condition) {


Just for separation of concerns, I would put this in a single file.

jeeyyy · 2018-12-17T15:36:32Z

lib/commons/matches/from-function.js

+
+	// Check that the property has all the expected values
+	return Object.keys(matcher).every(propName => {
+		return matches.fromString(getValue(propName), matcher[propName]);


Was this addressed @WilcoFiers?

jeeyyy · 2018-12-17T15:39:06Z

lib/commons/matches/index.js

@@ -0,0 +1,51 @@
+/* exported matches */


We have introduced a npm script npm run api-docs, which generates jsdoc's based API documentation. This can be accessed via http://localhost/doc/api.

See - https://github.com/dequelabs/axe-core/blob/develop/doc/API.md#full-api-reference-for-developers

jeeyyy · 2018-12-17T15:40:10Z

lib/commons/matches/node-name.js

+	if (typeof isXHTMLGlobal === 'undefined') {
+		isXHTMLGlobal = axe.utils.isXHTML(node.ownerDocument);
+	}
+	if (typeof isXHTML === 'undefined') {


Was this addressed @WilcoFiers?

jeeyyy · 2018-12-17T15:44:41Z

test/commons/matches/from-definition.js

+    }))
+  })
+
+  it('matches a definition with an `properties` property', function () {


editorial nitpick definition with a properties property.

jeeyyy · 2018-12-17T15:45:55Z

test/commons/matches/from-definition.js

+  it('returns true when all matching properties return true', function () {
+    fixture.innerHTML = '<input value="bar" aria-disabled="true" />'
+    assert.isTrue(fromDefinition(fixture.firstChild, {
+      nodeName: 'input',


👍 Loving the api. Very nice.

jeeyyy · 2018-12-17T15:54:39Z

test/commons/matches/from-function.js

+    })
+  })
+
+  describe('with void matchers', function () {


what is the thinking behind allowing null or undefined in .fromFunction matcher?

Probably wasn't necessary. I'm going to remove it.

marcysutton · 2018-12-18T02:50:37Z

lib/commons/aria/index.js

+			{
+				nodeName: 'img',
+				attributes: {
+					alt: isNotNull


How does this work with empty quotes? Or is it just existence of the attribute, irrespective of values?

This matches an element without alt. If you want to match an element with an empty alt, do attributes: { alt : '' }. Looking back at the spec, I think that's what it should have been. Good find.

Edit: OK so I looked into this lookupTable.evaluateRoleForElement is handling the empty alt scenario. It's a bit wonky. I do think we should change that, but I'd prefer to keep that out of this PR.

@WilcoFiers can you file a new issue for that?

marcysutton · 2018-12-18T03:11:53Z

lib/commons/matches/attributes.js

@@ -0,0 +1,8 @@
+/* global matches */
+matches.attributes = function matchesAttributes (node, matcher) {


Where/how does this method get called?

marcysutton · 2018-12-18T03:14:30Z

lib/commons/matches/index.js

+ * Example:
+ * ```js
+ * // Match a single nodeName:
+ * axe.utils.elementMatch(elm, 'div')


This seems outdated–should it read matches instead of elementMatch?

Right you are.

marcysutton · 2018-12-18T03:16:42Z

lib/commons/matches/from-definition.js

+matches.fromDefinition = function matchFromDefinition (node, definition) {
+  node = node.actualNode || node;
+	if (Array.isArray(definition)) {
+		return definition.some(definition => matches(node, definition));


The reuse of the word definition as a variable in .some makes the recursive call a bit difficult to follow. Can you refine it to describe more accurately what is being provided as an argument in the recursive call to matches?

marcysutton · 2018-12-18T03:21:00Z

lib/commons/matches/from-definition.js

+    if (!matchers.includes(matcherName)) {
+      throw new Error(`Unknown matcher type "${matcherName}"`)
+    }
+    const matchMethod = matches[matcherName]


Can you add a comment here about what is happening with matches? It's so overloaded it's hard to tell exactly.

Yeah, that was the challenge writing this code. I'll add in comments.

marcysutton · 2018-12-18T03:22:15Z

lib/commons/matches/from-definition.js

+  })
+}
+
+matches.condition = function (node, condition) {


It definitely needs some comments about usage. Similar to matches.attributes, it's difficult to follow how the lookupTable functions with this matches object...

marcysutton · 2018-12-18T03:27:06Z

lib/commons/matches/from-function.js

+ * Check if the value from a function matches some condition
+ * 
+ * Each key on the matcher object is passed to getValue, the returned value
+ * must than match the with the value of that matcher


This text could be improved: Each key on the matcher object is passed to getValue, the returned value must match with the value of that matcher

It would also benefit from an example like the other methods.

marcysutton · 2018-12-18T03:32:07Z

lib/commons/matches/from-function.js

+		return true;
+	}
+	if (matcherType !== 'object' || Array.isArray(matcher) || matcher instanceof RegExp) {
+		throw new Error('Expect elementMatch properties to be an object');


Is elementMatch correct here?

marcysutton · 2018-12-18T03:35:44Z

lib/commons/matches/properties.js

@@ -0,0 +1,9 @@
+/* global matches */
+matches.properties = function matchesProperties (node, matcher) {


JSDoc comments would be good.

I've processed all your comments. Please look again.

jeeyyy

👍

marcysutton · 2018-12-26T18:29:44Z

lib/commons/aria/index.js

+			{
+				nodeName: 'img',
+				attributes: {
+					alt: isNotNull


@WilcoFiers can you file a new issue for that?

marcysutton · 2018-12-26T18:36:46Z

lib/commons/matches/index.js

@@ -0,0 +1,51 @@
+/* exported matches */


IMO commons functions should all be documented, especially with the increasing team collaboration coming up.

feat(commons): Add matches methods

f40d58c

WilcoFiers requested a review from jeeyyy December 9, 2018 17:49

WilcoFiers requested a review from a team as a code owner December 9, 2018 17:49

jeeyyy previously requested changes Dec 10, 2018

View reviewed changes

dylanb reviewed Dec 11, 2018

View reviewed changes

chore: Updated matches based on PR review

df9236e

chore: dummy update

98884f1

jeeyyy previously requested changes Dec 17, 2018

View reviewed changes

marcysutton previously requested changes Dec 18, 2018

View reviewed changes

WilcoFiers added 3 commits December 18, 2018 14:23

chore: Update based on PR feedback

eba87eb

chore: rerun prettier

90847f9

chore: fix space

d016ffb

jeeyyy previously approved these changes Dec 19, 2018

View reviewed changes

WilcoFiers dismissed jeeyyy’s stale review via 3a065f0 December 20, 2018 10:57

chore: Rename fromString to fromPrimative

cb5b1ae

WilcoFiers force-pushed the matches branch from 3a065f0 to cb5b1ae Compare December 20, 2018 10:58

marcysutton approved these changes Dec 26, 2018

View reviewed changes

Merge branch 'develop' into matches

ead36e3

WilcoFiers merged commit 986c97a into develop Jan 3, 2019

WilcoFiers deleted the matches branch January 3, 2019 11:51

		@@ -0,0 +1,8 @@
		/* global matches */
		matches.attributes = function matchesAttributes (node, matcher) {

		@@ -0,0 +1,15 @@
		/* global matches */
		matches.fromFunction = function matchFromFunction(getValue, matcher) {

		@@ -0,0 +1,9 @@
		/* global matches */
		matches.properties = function matchesProperties (node, matcher) {

feat(commons): Add matches methods #1270

feat(commons): Add matches methods #1270

Conversation

WilcoFiers commented Dec 9, 2018 • edited by jeeyyy Loading

Reviewer checks

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

Choose a reason for hiding this comment

marcysutton Dec 18, 2018 • edited Loading

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

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

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

dylanb 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

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

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

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 Dec 18, 2018 • edited Loading

Choose a reason for hiding this comment

WilcoFiers Dec 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcysutton Dec 18, 2018 • edited Loading

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

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 Dec 9, 2018 •

edited by jeeyyy

Loading

marcysutton Dec 18, 2018 •

edited

Loading

marcysutton Dec 18, 2018 •

edited

Loading

WilcoFiers Dec 18, 2018 •

edited

Loading

marcysutton Dec 18, 2018 •

edited

Loading