976082 - [AccessFu] Provide tests for touch adapter.

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Yura Zenevich [:yzen] (please use "needinfo")]

We need to write tests for the touch adapter and all the gestures it supports.

Comment 1

[Yura Zenevich [:yzen] (please use "needinfo")]

Attachment: 976082 patch v1

Added tests for touch adapter, also jshinted the jsatcommon and other files I touched.

Comment 2

[Eitan Isaacson [:eeejay]]

Comment on attachment 8385623 [details] [diff] [review]
976082 patch v1

Review of attachment 8385623 [details] [diff] [review]:
-----------------------------------------------------------------

Overall, looks good! Mostly need clarifications and readability. I would re-think and document well the structures in gestures.js to make it very simple and easy to add tests from captured data, like we talked about.

::: accessible/tests/mochitest/jsat/dom_helper.js
@@ +12,5 @@
> +
> +var winUtils = win.QueryInterface(Ci.nsIInterfaceRequestor).getInterface(
> +  Ci.nsIDOMWindowUtils);
> +
> +function getCoordinates(aTarget) {

It took me a while to understand what this function does. I think that
(a) it is not named appropriately.
(b) aTarget is a misleading name, i thought it was a dom event target (ie. an element).
(c) the items in aTarget need to have their structure documented, x and y are offsets in inches from the center of "base"?

@@ +25,5 @@
> +      x: Math.round(bounds[0] - parentBounds[0] + bounds[2] / 2 +
> +        target.x * dpi),
> +      y: Math.round(bounds[1] - parentBounds[1] + bounds[3] / 2 +
> +        target.y * dpi)
> +    });

You may enjoy doing all sorts of geometry operations with geometry.jsm.
https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/Geometry.jsm/Point

I'm fine either way.

@@ +41,5 @@
> +    for (var i = 0; i < coords.length; ++i) {
> +      var {x, y} = coords[i];
> +      var node = document.elementFromPoint(x, y);
> +      var touch = document.createTouch(window, node, aName === 'touchstart' ?
> +        1 : touchList.item(i).identifier, x, y, x, y);

Don't you only want to create a touch if aName === 'touchstart', and in other cases just pull it out of touchList?

@@ +85,5 @@
> +    function fireEvent() {
> +      eventMap[step.type](step.target, step.type);
> +    }
> +    function runStep() {
> +      if (next && next.expectedGestures) {

Why not just have an expectedGesture on the current step?

@@ +102,5 @@
> +      }
> +    });
> +  });
> +  AccessFuTest.addFunc(function() {
> +    window.setTimeout(AccessFuTest.nextTest, 1000);

What is this for? Add a comment if it is necessary.

::: accessible/tests/mochitest/jsat/gestures.js
@@ +1,5 @@
> +'use strict';
> +
> +/* exported gestures*/
> +
> +var gestures = [

How about a JSON file?

::: accessible/tests/mochitest/jsat/jsatcommon.js
@@ +125,4 @@
>        return;
>      }
>      // Create an Iterator for gTestFuncs array.
> +    gIterator = Iterator(gTestFuncs); // jshint ignore:line

What error are you supressing?

Comment 3

[Yura Zenevich [:yzen] (please use "needinfo")]

(In reply to Eitan Isaacson [:eeejay] from comment #2)
> Comment on attachment 8385623 [details] [diff] [review]
> 976082 patch v1
> 
> @@ +125,4 @@
> >        return;
> >      }
> >      // Create an Iterator for gTestFuncs array.
> > +    gIterator = Iterator(gTestFuncs); // jshint ignore:line
> 
> What error are you supressing?
JSHint complains about a missing |new| in front of the Iterator, which is not necessary there.

Comment 4

[Eitan Isaacson [:eeejay]]

One more thing..

> target.x = target.x || 0;
> target.y = target.y || 0;

On principal, I think we should avoid modifying argument data unless it is explicitly an out arg, so I would not assign this back to |target|.

Comment 5

[Yura Zenevich [:yzen] (please use "needinfo")]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e91a61089c37

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

This seems to have failed every single time it ran mochitest-oth on just Linux debug builds: https://tbpl.mozilla.org/php/getParsedLog.php?id=35689281&tree=Mozilla-Inbound

Backed out in http://hg.mozilla.org/integration/mozilla-inbound/rev/cdb8fc90adff

Comment 7

[Yura Zenevich [:yzen] (please use "needinfo")]

https://tbpl.mozilla.org/?tree=Try&rev=f8b98f1b7932

Comment 8

[Yura Zenevich [:yzen] (please use "needinfo")]

https://tbpl.mozilla.org/?tree=Try&rev=0490412e7172

Comment 9

[Yura Zenevich [:yzen] (please use "needinfo")]

Attachment: 976082 patch v2

Some improvements and speed up in the test harness.

Comment 10

[Yura Zenevich [:yzen] (please use "needinfo")]

Attachment: 976082 patch v3

Making timeouts more reliable.
https://tbpl.mozilla.org/?tree=Try&rev=b9612f51a515

Comment 11

[Eitan Isaacson [:eeejay]]

Comment on attachment 8388665 [details] [diff] [review]
976082 patch v3

Review of attachment 8388665 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.

Comment 12

[Yura Zenevich [:yzen] (please use "needinfo")]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a7882947aea9

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a7882947aea9