Closed Bug 976082 Opened 6 years ago Closed 6 years ago

[AccessFu] Provide tests for touch adapter.

Categories

(Core :: Disability Access APIs, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: yzen, Assigned: yzen)

References

Details

Attachments

(1 file, 2 obsolete files)

We need to write tests for the touch adapter and all the gestures it supports.
Attached patch 976082 patch v1 (obsolete) — Splinter Review
Added tests for touch adapter, also jshinted the jsatcommon and other files I touched.
Attachment #8385623 - Flags: review?(eitan)
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?
Attachment #8385623 - Flags: review?(eitan) → review+
(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.
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|.
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
Flags: needinfo?(yzenevich)
Blocks: 981015
Attached patch 976082 patch v2 (obsolete) — Splinter Review
Some improvements and speed up in the test harness.
Attachment #8385623 - Attachment is obsolete: true
Attachment #8388526 - Flags: review?(eitan)
Attachment #8388526 - Flags: review?(eitan)
Attached patch 976082 patch v3Splinter Review
Making timeouts more reliable.
https://tbpl.mozilla.org/?tree=Try&rev=b9612f51a515
Attachment #8388526 - Attachment is obsolete: true
Attachment #8388665 - Flags: review?(eitan)
Comment on attachment 8388665 [details] [diff] [review]
976082 patch v3

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

Looks good.
Attachment #8388665 - Flags: review+
Attachment #8388665 - Flags: review?(eitan)
https://hg.mozilla.org/mozilla-central/rev/a7882947aea9
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in before you can comment on or make changes to this bug.