Enhance Mozelement with touch events

RESOLVED FIXED

Status

P1
normal
RESOLVED FIXED
6 years ago
2 years ago

People

(Reporter: andrei, Assigned: andrei)

Tracking

Details

(Whiteboard: [metro][mozmill-2.0+][ateamtrack: p=mozmill q=2013q3 m=2])

Attachments

(1 attachment, 8 obsolete attachments)

(Assignee)

Description

6 years ago
For working with metro we need to be able to handle touch events.
Once bug 874895 lands we should also add support for basic touch events in Mozelement.

These basic events should mirror the current click events.

We should probably have:
- tap
- doubletap // do we use something like this?
- waitThenTap // helper method, similar to waitThenClick
- touchStart // alternative names? touchDown / touchUp
- touchEnd

After basic support we will need to add support for more advanced gestures:
(do we have a list of supported gestures by Firefox Metro ?)
- swipe // we might need to also define swipeFromBottom / swipeFromTop as helpers

(these might not be used ATM)
- flick
- pinch // in/out
(Assignee)

Updated

6 years ago
Blocks: 879382
Whiteboard: [metro] → [metro][mozmill-2.0?]
(Assignee)

Comment 1

6 years ago
Created attachment 759526 [details] [diff] [review]
First Part v1

Basic support for touch events.

Some thoughts:
1) We miss some helper methods in EventUtils.js (namely: synthesizeTouchExpectEvent) Henrik suggested we might want to patch the EventUtils.js from mozilla-central

2) There seems to be no "preferred" way of doing toubleTap. I am sending 2 tap identical tap events one after the other. Should there be a better way of achieving that?

3) In mutt tests where we test basic click functionality, I duplicated some of those tests to have some coverage over touch events. However in expected_events.js the following check is failing:

> expect.doesNotThrow(function () {
>   fname.tap(2, 2, {type: "focus"});
> }, "Error", "tap() on a text field raises a focus event.");

Whilst the similar one for click() is passing.

========

I've done more testing on an Acer Win8 Tablet/PC (horribly crappy machine).
At the moment Firefox is sending regular Mouse events alongside each Touch event
(for any touchstart/touchend there is a mousedown/mouseup, for touchmove there is a mousemove).

So it won't impact us ATM not to be fully touch compliant, we can defer to regular Mouse events when needed.

I will have an update which fallbacks on Mouse events where needed.
Assignee: nobody → andrei.eftimie
Status: NEW → ASSIGNED
Attachment #759526 - Flags: feedback?(hskupin)
Attachment #759526 - Flags: feedback?(dave.hunt)
Attachment #759526 - Flags: feedback?(andreea.matei)
Comment on attachment 759526 [details] [diff] [review]
First Part v1

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

As mentioned this will be changed, so we don't have to add our own methods to EventUtils.js. Instead we will take the methods which are in use by metro mochitests:
http://mxr.mozilla.org/mozilla-central/source/browser/metro/base/tests/mochitest/head.js

That said, I will remove the feedback request.
Attachment #759526 - Flags: feedback?(hskupin)
Attachment #759526 - Flags: feedback?(dave.hunt)
Attachment #759526 - Flags: feedback?(andreea.matei)
(Assignee)

Updated

6 years ago
Blocks: 880760

Updated

6 years ago
Whiteboard: [metro][mozmill-2.0?] → [metro][mozmill-2.0+]
(Assignee)

Comment 3

6 years ago
Created attachment 760976 [details] [diff] [review]
patch v2

This has:

- tap
- doubleTap
- longPress
- move
- press // touchstart
- release // touchend
- touchmove

Code is inspired by Mochitests (referenced in comment 2)
Method names are inspired from Marionette

The expectedEvent is only available on tap and doubleTap (they use synthesizeMouse).
The rest use synthesizeTouch which is missing its synthesizeTouchExpectEvent from EventUtils.js so we don't have that argument.

Should we try to find a way to implement that?
Attachment #759526 - Attachment is obsolete: true
Attachment #760976 - Flags: review?(hskupin)

Updated

6 years ago
Whiteboard: [metro][mozmill-2.0+] → [metro][mozmill-2.0+][sprint2013-37]
Individual tests should better be marked as dependent on bug 879371.
Blocks: 879371
No longer blocks: 731280, 877131, 879382, 880760
(In reply to Andrei Eftimie from comment #3)
> The expectedEvent is only available on tap and doubleTap (they use
> synthesizeMouse).
> The rest use synthesizeTouch which is missing its synthesizeTouchExpectEvent
> from EventUtils.js so we don't have that argument.
> 
> Should we try to find a way to implement that?

That's most likely not on our side, but you should file a bug for it.
Comment on attachment 760976 [details] [diff] [review]
patch v2

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

This looks great and not that complicated. Lets get the remaining issues sorted out today, so we can get it landed and RC4 released.

::: mozmill/mozmill/extension/resource/driver/mozelement.js
@@ +375,5 @@
>  
> +/**
> + * Synthesize a touch tap event on the given element
> + */
> +MozMillElement.prototype.tap = function (aX, aY, expectedEvent) {

We should adapt the naming of the x and y coordinates from other already existent methods like click(). Don't invent new names here, or adjust them all. Also use the 'a' prefix everywhere, and extend the jsdoc for all parameters used here. This also applies to other methods.

@@ +404,5 @@
> +/**
> + * Synthesize a long press
> + */
> +MozMillElement.prototype.longPress = function (aX, aY, aTime) {
> +  var time = aTime || 500;

Is 500ms enough to bring up the selection controls on text elements? I would rather go with a larger default value e.g. 1s.

@@ +407,5 @@
> +MozMillElement.prototype.longPress = function (aX, aY, aTime) {
> +  var time = aTime || 500;
> +  EventUtils.synthesizeTouchAtPoint(aX, aY, { type: "touchstart" });
> +  controller.sleep(time);
> +  EventUtils.synthesizeTouchAtPoint(aX, aY, { type: "touchend" });

Don't we need Ci.nsIDOMMouseEvent.MOZ_SOURCE_TOUCH here? Is it used by default in the called method?

@@ +417,5 @@
> +
> +/**
> + * Synthesize a move
> + */
> +MozMillElement.prototype.move = function (aStartX, aStartY, aEndX, aEndY) {

This is misleading when attached to the mozelement class. You might think that you can move the element. Most likely one reason why the events shouldn't have been attached to it at all. For now we might have to adjust the name a little bit.

Can we get a discussion started on that topic in the mozmill mailing list? I think we should partly refactor this before 2.0 goes out. A separate class for events might be very helpful.

@@ +420,5 @@
> + */
> +MozMillElement.prototype.move = function (aStartX, aStartY, aEndX, aEndY) {
> +  EventUtils.synthesizeTouchAtPoint(aStartX, aStartY, { type: "touchstart" });
> +  EventUtils.synthesizeTouchAtPoint(aEndX, aEndY, { type: "touchmove" });
> +  EventUtils.synthesizeTouchAtPoint(aEndX, aEndY, { type: "touchend" });

I would call  press, move, release here. Saying it should be a wrapper around the other three new methods.

@@ +452,5 @@
> +
> +/**
> + * Synthesize a touchMove event on the given element
> + */
> +MozMillElement.prototype.touchMove = function (aX, aY) {

Probably a good idea to prefix all events with 'touch'. Shall we call this drag? It would lessen the confusion with the above move method.

::: mutt/mutt/tests/js/controller/synthesize_events.js
@@ +23,5 @@
>  
> +  // Tapping the search field should raise a focus event
> +  fname.tap();
> +  expect.equal(fname.getNode(), controller.tabs.activeTab.activeElement,
> +               "tap() put the focus to the element.");

Please remove that action and check here and put together a new test file which we can stick under js/metro, specifically for touch events.
Attachment #760976 - Flags: review?(hskupin) → review-
(Assignee)

Comment 7

6 years ago
Created attachment 762646 [details] [diff] [review]
patch v3

(In reply to Henrik Skupin (:whimboo) from comment #6)
> ::: mozmill/mozmill/extension/resource/driver/mozelement.js
> @@ +375,5 @@
> >
> > +/**
> > + * Synthesize a touch tap event on the given element
> > + */
> > +MozMillElement.prototype.tap = function (aX, aY, expectedEvent) {
>
> We should adapt the naming of the x and y coordinates from other already
> existent methods like click(). Don't invent new names here, or adjust them
> all. Also use the 'a' prefix everywhere, and extend the jsdoc for all
> parameters used here. This also applies to other methods.

EventUtils uses a mix of left/top (3 instances) and offsetX/offsetY (4 instances)
Mochitests seem to use offset:
http://mxr.mozilla.org/mozilla-central/source/browser/metro/base/tests/mochitest/head.js#453
And Marionette seems to be using x/y:
http://mxr.mozilla.org/mozilla-central/source/testing/marionette/client/marionette/marionette.py#108 (this is where I initially took the naming from)

left/top seems to be used  more in lieu with CSS, and offsetX/offsetY (or directly x/y) with JS
For consistency I'm changing it all to offsetX/offsetY

Also added jsdoc for all new action methods.
Wondering if we should add them to all methods.
We have bug 882137 for that but that only applies to mozmill-tests
Still out of the scope of this bug, we should probably file another one to get this done.

> @@ +404,5 @@
> > +/**
> > + * Synthesize a long press
> > + */
> > +MozMillElement.prototype.longPress = function (aX, aY, aTime) {
> > +  var time = aTime || 500;
>
> Is 500ms enough to bring up the selection controls on text elements? I would
> rather go with a larger default value e.g. 1s.

I took the 500ms from how long it takes a touchevent to become a press from here:
http://mxr.mozilla.org/mozilla-central/source/accessible/src/jsat/TouchAdapter.jsm#34

But as you are saying, Marionette seems to also use 1s:
http://mxr.mozilla.org/mozilla-central/source/testing/marionette/marionette-listener.js#849

And it does seem more reliable then to go with the minimum.
Changed it to 1s

> @@ +407,5 @@
> > +MozMillElement.prototype.longPress = function (aX, aY, aTime) {
> > +  var time = aTime || 500;
> > +  EventUtils.synthesizeTouchAtPoint(aX, aY, { type: "touchstart" });
> > +  controller.sleep(time);
> > +  EventUtils.synthesizeTouchAtPoint(aX, aY, { type: "touchend" });
>
> Don't we need Ci.nsIDOMMouseEvent.MOZ_SOURCE_TOUCH here? Is it used by
> default in the called method?

We used MOZ_SOURCE_TOUCH where we trigger Mouse events.
See mochi tap/doubleTap:
http://mxr.mozilla.org/mozilla-central/source/browser/metro/base/tests/mochitest/head.js#561

It doesn't seem to be needed when using real touch events.
See mochi touchDrag (move) event:
http://mxr.mozilla.org/mozilla-central/source/browser/metro/base/tests/mochitest/head.js#584

> @@ +417,5 @@
> > +
> > +/**
> > + * Synthesize a move
> > + */
> > +MozMillElement.prototype.move = function (aStartX, aStartY, aEndX, aEndY) {
>
> This is misleading when attached to the mozelement class. You might think
> that you can move the element. Most likely one reason why the events
> shouldn't have been attached to it at all. For now we might have to adjust
> the name a little bit.
>
> Can we get a discussion started on that topic in the mozmill mailing list? I
> think we should partly refactor this before 2.0 goes out. A separate class
> for events might be very helpful.

I agree. Calling Button.move(...) does seem it might actually move the element, not trigger a user touchDrag event. Sent out a message on the mailing list asking people for feedback.

> @@ +420,5 @@
> > + */
> > +MozMillElement.prototype.move = function (aStartX, aStartY, aEndX, aEndY) {
> > +  EventUtils.synthesizeTouchAtPoint(aStartX, aStartY, { type: "touchstart" });
> > +  EventUtils.synthesizeTouchAtPoint(aEndX, aEndY, { type: "touchmove" });
> > +  EventUtils.synthesizeTouchAtPoint(aEndX, aEndY, { type: "touchend" });
>
> I would call  press, move, release here. Saying it should be a wrapper
> around the other three new methods.

Good point, done.

> @@ +452,5 @@
> > +
> > +/**
> > + * Synthesize a touchMove event on the given element
> > + */
> > +MozMillElement.prototype.touchMove = function (aX, aY) {
>
> Probably a good idea to prefix all events with 'touch'. Shall we call this
> drag? It would lessen the confusion with the above move method.

This is not actually a drag. This calls the native touchmove event.
We should then probably rename the move method above to touchDrag

I agree in prefixing them with touch, and since these are all proxies to native touch events, I suggest to name them the same (touchStart, touchEnd, touchMove).
Or better with no camelCase to be identical to the native ones: touchstart, touchend, touchmove

We will not be Marionette compatible (naming wise) this way, but it will be clearer

> ::: mutt/mutt/tests/js/controller/synthesize_events.js
> @@ +23,5 @@
> >
> > +  // Tapping the search field should raise a focus event
> > +  fname.tap();
> > +  expect.equal(fname.getNode(), controller.tabs.activeTab.activeElement,
> > +               "tap() put the focus to the element.");
>
> Please remove that action and check here and put together a new test file
> which we can stick under js/metro, specifically for touch events.

Done that.
We should probably have at least 1 test for each event.

As a disclaimer ATM we only really tested tap that it works (which uses mouseEvents not native touch ones). I'll work on these now.
Attachment #760976 - Attachment is obsolete: true
Attachment #762646 - Flags: review?(hskupin)
Comment on attachment 762646 [details] [diff] [review]
patch v3

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

::: mozmill/mozmill/extension/resource/driver/mozelement.js
@@ +406,5 @@
> + * @param {Number} [aOffsetY]
> + *        Top offset in px where the event is triggered
> + * @param {Object} [aExpectedEvent]
> + *        Information about the expected event to occur
> + * @param {ElemBase} [aExpectedEvent.target=this.element]

ElemBase doesn't exist anymore. We have to replace it with MozMillElement.

@@ +547,5 @@
> + *        Time to wait for the element to be available
> + * @param {Number} [aInterval=100]
> + *        Interval to check for availability
> + * @param {[type]} aOffsetX
> + *        Left offset where the event is triggered

Are those optional or required? You mixed something up here. If those are required please move them to the front.

@@ +559,5 @@
> + *        Type of the expected mouse event
> + */
> +MozMillElement.prototype.waitThenTap = function (aTimeout, aInterval,
> +                                                 aOffsetX, aOffsetY, aExpectedEvent) {
> +  this.waitForElement(aTimeout, aInterval);

I would rather do a MozMillElement.waitForElement(timeout, delay).tap(). Something we would only have to do is to return 'this' within waitForElement().

::: mutt/mutt/tests/js/metro/testTouchEvents.js
@@ +13,5 @@
> +  controller.open(TEST_DATA);
> +  controller.waitForPageLoad();
> +
> +  let fname = new elementslib.ID(controller.tabs.activeTab, "fname");
> +  let lname = new elementslib.ID(controller.tabs.activeTab, "lname");

Please use findElement() to retrieve the elements.

@@ +24,5 @@
> +    lname.tap(2, 2, {type: "keypress"});
> +  }, "Error", "tap() on a text field does not raise a keypress event.");
> +
> +  // Tapping the search field should raise a focus event
> +  fname.tap();

This does not tap on the search field. Also I don't see the necessity of this following check. You can already do that for the tap above.
Attachment #762646 - Flags: review?(hskupin) → review-
(Assignee)

Comment 9

6 years ago
Created attachment 762700 [details] [diff] [review]
patch v4

Fixed nits.
Attachment #762646 - Attachment is obsolete: true
Attachment #762700 - Flags: review?(hskupin)
Comment on attachment 762700 [details] [diff] [review]
patch v4

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

As spoken yesterday on IRC I would like to see some additional tests first before I want to do a review.
Attachment #762700 - Flags: review?(hskupin)
Andrei, can we please get an update here? We should really get this into rc5!
(Assignee)

Comment 12

6 years ago
Created attachment 779109 [details] [diff] [review]
0001-Bug-880426-Added-touch-events-support-for-Mozelement.patch

We needed some tests for the new supported touch events.
I've had a really tough time getting any tests that rely specifically on touch.

We have here one for tap, doubletap, and waitThenTap.
We are missing touchstart, touchmove, touchend, longpress.

What we discussed is that because we don't have support for expectedEvent, we can't reliably test these events occuring, so we might as well test their outcome.
I had several failed attempts at generating these tests.

The biggest problem I had is missing a real touch device to see which events occur on some interactions. Most of the interactions I've tested (while in MV, on the Acer/Asus Tablet) trigger both touch and regular mouse events, and FF might require a combination of both.

What I've tried: 
- making a clone of dragToElement but for touch, so we can touchDrag an element
- trying to drag selection monocles
- issuing a longpress to get the contextmenu (not good, marionette explicitly triggers an openContextMenu event when issuing longpress)

=====

We here want to test that the correct events have been triggered.
We might aswell patch EventUtils.js to handle that, or talk to the devs that maintain EventUtils to include expectedEvents for touch.

If we want to get this in mozmill soon, we might as well do it without a full test coverage, and file a bug to add those tests when support is more mature (or if we find some workarounds).

Let me know if this sounds acceptable, or if you have other ideas.
Thanks
Attachment #762700 - Attachment is obsolete: true
Attachment #779109 - Flags: review?(hskupin)
Attachment #779109 - Flags: review?(dave.hunt)
Comment on attachment 779109 [details] [diff] [review]
0001-Bug-880426-Added-touch-events-support-for-Mozelement.patch

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

> The biggest problem I had is missing a real touch device to see which events
> occur on some interactions. Most of the interactions I've tested (while in
> MV, on the Acer/Asus Tablet) trigger both touch and regular mouse events,
> and FF might require a combination of both.

What's the status of this? I thought we had some devices on order.

::: mutt/mutt/tests/js/metro/tests.ini
@@ +2,4 @@
>  type = javascript
>  
>  [testTabs.js]
> +[include:touchEvents/tests.ini]

Rename the folder to testTouchEvents in accordance with the work on bug 767286

::: mutt/mutt/tests/js/metro/touchEvents/tests.ini
@@ +1,1 @@
> +[DEFAULT]

Please name this file manifest.ini in accordance with the work on bug 767286

@@ +1,4 @@
> +[DEFAULT]
> +type = javascript
> +
> +[tap.js]

Rename to testTab.js

@@ +1,5 @@
> +[DEFAULT]
> +type = javascript
> +
> +[tap.js]
> +[doubleTap.js]

Rename to testDoubleTap.js

@@ +2,5 @@
> +type = javascript
> +
> +[tap.js]
> +[doubleTap.js]
> +[waitThenTap.js]

Rename to testWaitThenTap.js
Attachment #779109 - Flags: review?(dave.hunt) → review-
(Assignee)

Comment 14

6 years ago
Created attachment 780460 [details] [diff] [review]
0001-Bug-880426-Added-touch-events-support-for-Mozelement.patch

Renamed files to match new standards.

> What's the status of this? I thought we had some devices on order.
Not yet. I think this is waiting on Clint. Not sure about a status.
Attachment #779109 - Attachment is obsolete: true
Attachment #779109 - Flags: review?(hskupin)
Attachment #780460 - Flags: review?(hskupin)
Attachment #780460 - Flags: review?(dave.hunt)
(In reply to Andrei Eftimie from comment #14)
> > What's the status of this? I thought we had some devices on order.
> Not yet. I think this is waiting on Clint. Not sure about a status.

Those have been already ordered. We have to wait for their delivery now. It can take a while I assume.
Comment on attachment 780460 [details] [diff] [review]
0001-Bug-880426-Added-touch-events-support-for-Mozelement.patch

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

Wow, lots of good work here since I last checked. There is not that much left to do. So lets get this done. Thanks Andrei!

::: mozmill/mozmill/extension/resource/driver/mozelement.js
@@ +354,4 @@
>  
>    var rect = this.element.getBoundingClientRect();
>  
> +  if (isNaN(aOffsetX) || aOffsetX === null) {

Good catch. Are there more instances of that across our code? Personally I would use: ´if (!aOffset || isNan(aOffsetX))´. That will also cover undefined.

@@ +487,5 @@
>  
> +/**
> + * Synthesize a general touch event on the given element
> + *
> + * @param {Number} [aOffsetX]

If parameters are optional you have to add the default value. That applies to nearly all parameters of that method.

@@ +505,5 @@
> + * @param {Boolean} [aEvent.ctrlKey]
> + *        A Boolean value indicating whether or not the control key was down
> + *        when the touch event was fired
> + * @param {Number} [aEvent.force=1]
> + * @param {Number} [aEvent.id=0]

No descriptions here?

@@ +510,5 @@
> + * @param {Boolean} [aEvent.metaKey]
> + *        A Boolean value indicating whether or not the meta key was down when
> + *        the touch event was fired.
> + * @param {Number} [aEvent.rx=1]
> + * @param {Number} [aEvent.ry=1]

No descriptions here?

@@ +568,5 @@
> + *        Left offset in px where the event is triggered
> + * @param {Number} [aOffsetY]
> + *        Top offset in px where the event is triggered
> + * @param {Object} [aExpectedEvent]
> + *        Information about the expected event to occur

This is not implemented yet, right? So please make a note here.

@@ +622,5 @@
> + *        Duration of the "press" event in ms
> + */
> +MozMillElement.prototype.longPress = function (aOffsetX, aOffsetY, aTime) {
> +  var time = aTime || 1000;
> +  this.touchStart(aOffsetX, aOffsetY);

nit: empty line before.

::: mutt/mutt/tests/js/metro/testTouchEvents/testDoubleTap.js
@@ +7,5 @@
> +const BASE_URL = collector.addHttpResource("../../_files/");
> +const TEST_DATA = BASE_URL + "complex.html";
> +const TEXT_SELECTION = "community";
> +
> +var setupModule = function (aModule) {

Let use ´function setupModule()´ and apply this to all other methods.

@@ +15,5 @@
> +var testTap = function () {
> +  controller.open(TEST_DATA);
> +  controller.waitForPageLoad();
> +
> +  // Greb the element and issue a doubleTap to select the text

nit 'Grab'

@@ +16,5 @@
> +  controller.open(TEST_DATA);
> +  controller.waitForPageLoad();
> +
> +  // Greb the element and issue a doubleTap to select the text
> +  var element = new findElement.Selector(controller.tabs.activeTab, "[name=community]");

Why don't you use findElement.Name()?

@@ +21,5 @@
> +  element.doubleTap();
> +
> +  // Check that the text has been properly selected
> +  var selection = controller.tabs.activeTabWindow.getSelection();
> +  expect.equal(selection.toString().toLowerCase(), TEXT_SELECTION,

Change TEXT_SELECTION to 'Community' instead of calling toLowerCase().

::: mutt/mutt/tests/js/metro/testTouchEvents/testTap.js
@@ +18,5 @@
> +  let lname = new findElement.ID(controller.tabs.activeTab, "lname");
> +
> +  expect.doesNotThrow(function () {
> +    lname.tap(null, null, {type: "focus"});
> +  }, "Error", "tap() on a text field raises a focus event.");

Please refresh my mind. Where does the Error exception comes from? Is that our code? If yes we should file a follow-up bug to get this changed to TypeError. As for now remove the quotes around it. There is no need anymore to specify it as a string.

::: mutt/mutt/tests/js/metro/testTouchEvents/testWaitThenTap.js
@@ +20,5 @@
> +  expect.doesNotThrow(function () {
> +    lname.waitThenTap(null, null, null, null, {type: "focus"});
> +  }, "Error", "tap() on a text field raises a focus event.");
> +
> +  // lname.blur();

What's with that line?
Attachment #780460 - Flags: review?(hskupin)
Attachment #780460 - Flags: review?(dave.hunt)
Attachment #780460 - Flags: review-
(Assignee)

Comment 17

6 years ago
Created attachment 781705 [details] [diff] [review]
7.patch

(In reply to Henrik Skupin (:whimboo) [away 07/29 - 08/11] from comment #16)
> Comment on attachment 780460 [details] [diff] [review]
> 0001-Bug-880426-Added-touch-events-support-for-Mozelement.patch
>
> Review of attachment 780460 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> Wow, lots of good work here since I last checked. There is not that much
> left to do. So lets get this done. Thanks Andrei!
>
> ::: mozmill/mozmill/extension/resource/driver/mozelement.js
> @@ +354,4 @@
> >
> >    var rect = this.element.getBoundingClientRect();
> >
> > +  if (isNaN(aOffsetX) || aOffsetX === null) {
>
> Good catch. Are there more instances of that across our code? Personally I
> would use: ´if (!aOffset || isNan(aOffsetX))´. That will also cover
> undefined.

Indeed better. Also found another instance.
Changed all of them.

> @@ +487,5 @@
> >
> > +/**
> > + * Synthesize a general touch event on the given element
> > + *
> > + * @param {Number} [aOffsetX]
>
> If parameters are optional you have to add the default value. That applies
> to nearly all parameters of that method.

Done

> @@ +505,5 @@
> > + * @param {Boolean} [aEvent.ctrlKey]
> > + *        A Boolean value indicating whether or not the control key was down
> > + *        when the touch event was fired
> > + * @param {Number} [aEvent.force=1]
> > + * @param {Number} [aEvent.id=0]
>
> No descriptions here?

Done

> @@ +510,5 @@
> > + * @param {Boolean} [aEvent.metaKey]
> > + *        A Boolean value indicating whether or not the meta key was down when
> > + *        the touch event was fired.
> > + * @param {Number} [aEvent.rx=1]
> > + * @param {Number} [aEvent.ry=1]
>
> No descriptions here?

Done

> @@ +568,5 @@
> > + *        Left offset in px where the event is triggered
> > + * @param {Number} [aOffsetY]
> > + *        Top offset in px where the event is triggered
> > + * @param {Object} [aExpectedEvent]
> > + *        Information about the expected event to occur
>
> This is not implemented yet, right? So please make a note here.

We are actually using mouseEvent for these. It is using the same code are
regular mouse events. We don't have it for 'real' touch events.

> @@ +622,5 @@
> > + *        Duration of the "press" event in ms
> > + */
> > +MozMillElement.prototype.longPress = function (aOffsetX, aOffsetY, aTime) {
> > +  var time = aTime || 1000;
> > +  this.touchStart(aOffsetX, aOffsetY);
>
> nit: empty line before.

Done

> ::: mutt/mutt/tests/js/metro/testTouchEvents/testDoubleTap.js
> @@ +7,5 @@
> > +const BASE_URL = collector.addHttpResource("../../_files/");
> > +const TEST_DATA = BASE_URL + "complex.html";
> > +const TEXT_SELECTION = "community";
> > +
> > +var setupModule = function (aModule) {
>
> Let use ´function setupModule()´ and apply this to all other methods.

Done

> @@ +15,5 @@
> > +var testTap = function () {
> > +  controller.open(TEST_DATA);
> > +  controller.waitForPageLoad();
> > +
> > +  // Greb the element and issue a doubleTap to select the text
>
> nit 'Grab'

Done

> @@ +16,5 @@
> > +  controller.open(TEST_DATA);
> > +  controller.waitForPageLoad();
> > +
> > +  // Greb the element and issue a doubleTap to select the text
> > +  var element = new findElement.Selector(controller.tabs.activeTab, "[name=community]");
>
> Why don't you use findElement.Name()?

Have not thought of using Name(). I'm too intimate with CSS Selectors :)
Done.

> @@ +21,5 @@
> > +  element.doubleTap();
> > +
> > +  // Check that the text has been properly selected
> > +  var selection = controller.tabs.activeTabWindow.getSelection();
> > +  expect.equal(selection.toString().toLowerCase(), TEXT_SELECTION,
>
> Change TEXT_SELECTION to 'Community' instead of calling toLowerCase().

Done

> ::: mutt/mutt/tests/js/metro/testTouchEvents/testTap.js
> @@ +18,5 @@
> > +  let lname = new findElement.ID(controller.tabs.activeTab, "lname");
> > +
> > +  expect.doesNotThrow(function () {
> > +    lname.tap(null, null, {type: "focus"});
> > +  }, "Error", "tap() on a text field raises a focus event.");
>
> Please refresh my mind. Where does the Error exception comes from? Is that
> our code? If yes we should file a follow-up bug to get this changed to
> TypeError. As for now remove the quotes around it. There is no need anymore
> to specify it as a string.

This was taken from 'expected_events.js'. Opened bug XXX for it.

> ::: mutt/mutt/tests/js/metro/testTouchEvents/testWaitThenTap.js
> @@ +20,5 @@
> > +  expect.doesNotThrow(function () {
> > +    lname.waitThenTap(null, null, null, null, {type: "focus"});
> > +  }, "Error", "tap() on a text field raises a focus event.");
> > +
> > +  // lname.blur();
>
> What's with that line?

Whoops. Fixed.
Attachment #780460 - Attachment is obsolete: true
Attachment #781705 - Flags: review?(hskupin)
Attachment #781705 - Flags: review?(dave.hunt)
Priority: -- → P1
Comment on attachment 781705 [details] [diff] [review]
7.patch

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

Great updates! Only some nits we want to fix before the patch can be landed. r=me with those nits fixed.

::: mozmill/mozmill/extension/resource/driver/mozelement.js
@@ +180,5 @@
>    };
>    var destRect = destNode.getBoundingClientRect();
>    var destCoords = {
> +    x: !aOffsetX || isNaN(aOffsetX) ? destRect.width / 2 : aOffsetX,
> +    y: !aOffsetY || isNaN(aOffsetY) ? destRect.height / 2 : aOffsetY

Please leave the brackets around the condition and the division. That will help a lot in the readability.

@@ +487,5 @@
>  
> +/**
> + * Synthesize a general touch event on the given element
> + *
> + * @param {Number} [aOffsetX=aElement.width/2]

I would leave the blanks around the operators. This also applies to other code changes in this patch.

@@ +542,5 @@
> + *        corresponds to the target of all the touches in the targetTouches
> + *        attribute, but note that other touches in this event may have a
> + *        different target. To be careful, you should use the target associated
> + *        with individual touches
> + */

Great explanations. It's huge but indeed very helpful for everyone who has to work with those methods.
Attachment #781705 - Flags: review?(hskupin)
Attachment #781705 - Flags: review?(dave.hunt)
Attachment #781705 - Flags: review+
(Assignee)

Comment 19

5 years ago
Created attachment 792807 [details] [diff] [review]
8.patch

> Great explanations. It's huge but indeed very helpful for everyone who has to 
> work with those methods.

I've just copied it from 2 sources from MDN (each had just half of them).

Fixed nits: brackets in the ternary operator and spaces around all operators that didn't had them

Also updated date for the commit :) was initial date from 6th June
Attachment #781705 - Attachment is obsolete: true
Attachment #792807 - Flags: review?(hskupin)
(Assignee)

Updated

5 years ago
Blocks: 879382
Comment on attachment 792807 [details] [diff] [review]
8.patch

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

Once again, please fix the remaining nits so we can get this landed.

::: mozmill/mozmill/extension/resource/driver/mozelement.js
@@ +487,5 @@
>  
> +/**
> + * Synthesize a general touch event on the given element
> + *
> + * @param {Number} [aOffsetX = aElement.width/2]

Not all spaces are fixed. We also need extra ones around the division operator. This applies to any other instance of that too.
Attachment #792807 - Flags: review?(hskupin) → review+
Comment on attachment 792807 [details] [diff] [review]
8.patch

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

Once again, please fix the remaining nits so we can get this landed.

::: mozmill/mozmill/extension/resource/driver/mozelement.js
@@ +487,5 @@
>  
> +/**
> + * Synthesize a general touch event on the given element
> + *
> + * @param {Number} [aOffsetX = aElement.width/2]

Oh, and there shouldn't be spaces around the equal sign:
http://code.google.com/p/jsdoc-toolkit/wiki/TagParam
(Assignee)

Comment 22

5 years ago
Created attachment 795382 [details] [diff] [review]
0001-Bug-880426-Added-touch-events-support-for-Mozelement.patch

Reverted space changes in jsdoc comments for the = operator and added space for the other operators.

As a sidenote: since jsdoc2 is no longer maintained since 2010 http://code.google.com/p/jsdoc-toolkit/
> NOTICE: As of 27 June 2010 the JsDoc Toolkit Version 2 project is no longer under active development or support.

Shouldn't we update our standard to use jsdoc 3?
https://github.com/jsdoc3/jsdoc
Attachment #792807 - Attachment is obsolete: true
Attachment #795382 - Flags: review?(hskupin)
Attachment #795382 - Flags: review?(dave.hunt)
(In reply to Andrei Eftimie from comment #22)
> Shouldn't we update our standard to use jsdoc 3?
> https://github.com/jsdoc3/jsdoc

This is something which does not belong to this bug. You might want to raise a mailing list thread or a new bug with details of changes and requirements. Then we can decide when we want to switch over if version 3 would work for us.
Comment on attachment 795382 [details] [diff] [review]
0001-Bug-880426-Added-touch-events-support-for-Mozelement.patch

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

Looks good now. Thanks Andrei!
Attachment #795382 - Flags: review?(hskupin)
Attachment #795382 - Flags: review?(dave.hunt)
Attachment #795382 - Flags: review+
Landed as:
https://github.com/mozilla/mozmill/commit/7e323d4b229be235caccc3375970827a8229b0a4
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [metro][mozmill-2.0+][sprint2013-37] → [metro][mozmill-2.0+][ateamtrack: p=mozmill q=2013q3 m=2]
(Assignee)

Updated

5 years ago
Blocks: 957547
Product: Testing → Testing Graveyard
You need to log in before you can comment on or make changes to this bug.