callback should follow error-first convention

[jasonkarns]

For better or for worse, nodejs conventions are taking over JavaScript as a whole, even in browser js.

One convention that applies to a11yCheck is the callback parameters. It currently accepts a single successful result parameter. NodeJS convention, by contrast, is well established that callbacks are defined as cb(error, result).

I strongly recommend that the a11yCheck callback conform to this pattern, as it would make it possible to use all the nodejs async utilities (like async itself, and any promise library with denodify-like support).

Some ready on node's error-first callbacks: http://thenodeway.io/posts/understanding-error-first-callbacks/

[dsturley]

I had thought of this previously, but never implemented it for a couple reasons:

1. It would break backwards compatibility.
2. Frames. If you specify a context that is inside a frame, we would need to make this determination inside the frame and then pass that back up to the initiating frame.

Due to the fact that it would be completely backwards incompatible, it is not likely that axe.a11yCheck will ever follow this pattern.

We could, however add a completely new method that follows Node conventions; but it would be a non-trivial effort to capture all the possible error conditions and ensure they are passed back alongside results.

[jasonkarns]

It would break backwards compatibility.

Yep, totally. A new method would be necessary, or at the very least, making the change only in a major release.

but it would be a non-trivial effort to capture all the possible error conditions and ensure they are passed back alongside results.

I'm not sure I follow this. In node, you usually get an error or results. Since you can't throw an exception when running async, the error parameter is a replacement for an exception. And, like normal error handling, you either have an exception or you have success. So I guess the question is: how are errors handled now? It can't be exceptions since it's async. And there isn't a parameter in the callback. So does that mean there currently isn't a way to have an error condition at all? Or is an error signified by an empty results object?

[dsturley]

It just logs errors to the console; which is not great, but at least you can see them. We purposely do not throw exceptions because of the nature of the DOM. Weird things can happen and prototypes can be clobbered, so it is important that a single failing Check doesn't stop execution.

A good example of an exception we can't really prevent is if a <form> has elements whose id maps to native DOM properties:

<form id="something">
<input name="id" type="hidden">
</form>

When rules that need to look at the ID of elements will choke when form.id is actually a reference to the hidden input.

The conditions which you pointed out would be useful:

1. No matching include
2. No matching rules (runOnly)

In addition there are a few more error conditions with frames:

1. When an iframe does not have axe injected. relevant code
2. When there is an error that prevents results from returning from a frame. relevant code
3. Weird Firefox condition where an iframe does not have a content window. relevant code

Additionally there are other exceptions that might occur with Rules and Checks:

1. Exceptions in Check's evaluate function. relevant code
2. Exceptions in a Check's custom matches function. (not caught)
3. Exceptions in a Rule's custom matches function. (not caught)

[jasonkarns]

Wanted to share another reason that would make the node-style preferable: promises.

Nearly every promise library out there has some form of denodeify method that can be used to seamlessly convert a callback-accepting function into a promise-returning function. These denodeify helpers only work, of course, when the callback follows the error-first node convention. Without the node-style callback, a11yCheck can't be converted to promises automatically, but must be done manually.

To the error handling side of things. Since a11yCheck is not currently throwing exceptions, it could continue to behave as it is now. That is, the callback could be converted to error-first style, but simply never call the callback with an error (for now). It would still be a BC-breaking change, but wouldn't require restructuring the error handling (yet).

[dylanb]

DECISION: We will implement this as a new API for doing the audit. The existing API will be built on top of this new API to ensure backwards compatibility. Still trying to determine the feasibility of cascading errors up from all the iframes.

[marcysutton]

This has been implemented with axe.run(). The axe.a11yCheck API is being deprecated.