839943 - Add support for touch action chains in marionette

Status: VERIFIED FIXED

(Remote Protocol :: Marionette, defect)

Description

[Malini Das [:mdas] - Away, not checking bugmail]

To synthesize any kind of gesture with any number of fingers, we want to employ action chains. For one finger gestures, we propose:
action_1 = Actions(marionette)
action_1.press(element).wait(5000).release(element)
action_1.perform()

To do a long press.

For a multifinger gesture, we can employ MultiActions. For example, say you want one finger to hold down while the other finger moves from one element to another, we propose doing this in the client:

multitouch = MultiActions(marionette)
action_1 = Actions(marionette)
action_2 = Actions(marionette)
action_1.press(element).move_to(element2).release(element2)
action_2.press(element3).wait().release(element3)
multitouch.add(action_1).add(action_2).perform()

We can have as many simultaneous action chains as we like, we just need to add them to the multiaction object. 

To implement this, imagine that actions_1 and actions_2 are two queues. You have to dequeue one action from both Actions objects and execute them at the same time, so we can have simulateous gestures. Using the example above:
1st dequeue:
[press(element), press(element3)]
2nd dequeue:
[move_to(element2), wait()]
**  note: wait() with no parameters means "I want to do nothing during this  set's execution". So you don't need to send any touch event for this  finger. It doesn't want to do anything. So the 2nd dequeue is really just:
[[move_to(element2)]
3rd dequeue:
[release(element2), release(element3)]

We have a few more examples and explanations over at https://etherpad.mozilla.org/touch-event-marionette.

This bug will track the initial client and server code to handle these action chains.

Comment 1

[Malini Das [:mdas] - Away, not checking bugmail]

*Note, we need to bump the marionette_client version number when we land this

Comment 2

[Malini Das [:mdas] - Away, not checking bugmail]

This bug will deal with the Actions object, and will also include work for bug 839940.

For the MultiActions object, see Bug 841101

Comment 3

[Ann(Yiming)]

Attachment: actionChain

Comment 4

[Ann(Yiming)]

Attachment: remove callback

Comment 5

[Malini Das [:mdas] - Away, not checking bugmail]

Comment on attachment 713635 [details] [diff] [review]
remove callback

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

Nice work, needs a few changes though:)

::: testing/marionette/client/marionette/marionette.py
@@ +124,5 @@
> +        self.action_chain.append(['move', element])
> +        return self
> +
> +    def move_by_offset(self, x, y):
> +        self.action_chain.append(['offset', x, y])

mind changing this to 'moveByOffset'? It's a little clearer, and that's the term we're hoping to use with the webdriver spec.

::: testing/marionette/client/marionette/tests/unit/test_single_finger.py
@@ +1,5 @@
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +import os

this is unused.

@@ +14,5 @@
> +        button = self.marionette.find_element("id", "mozLink")
> +        action = Actions(self.marionette)
> +        action.press(button).wait(5000).release(button)
> +        action.perform()
> +        time.sleep(5)

can you bump the sleep time in all these tests to 15 seconds? It may run quickly on your machine, but it'll probably lag in the test infrastructure.

::: testing/marionette/client/marionette/www/testAction.html
@@ +43,5 @@
> +  </div>
> +  <input name="myInput" type="text" value="asdf"/>
> +  <input name="myCheckBox" type="checkbox" />
> +  <h2 id="testh2" style="visibility: hidden" class="linkClass">Hidden</h2>
> +  <h3 id="testh3">Voluntary Termination</h3>

We don't really need these extra h3 tags, can you remove them?

::: testing/marionette/marionette-listener.js
@@ +921,5 @@
> +      globalEle = el;
> +      lastX = corx;
> +      lastY = cory;
> +      emitTouchEvent('touchstart', touch);
> +      i++;

instead of incrementing i here (and in the following cases), we can do it above of the switch block.

@@ +942,5 @@
> +      el = elementManager.getKnownElement(pack[1], curWindow);
> +      touch = createATouch(el, null, null, touchId);
> +      globalEle = el;
> +      lastX=0;
> +      lastY=0;

lastX and lastY should either both = null or = '50%', since in createATouch you check if they're equal to null, and if so, then '50%' is assigned to them.  Assigning them to 0 will mean we'll end up tapping (0,0) from the top-left of the element, ie: we'll be tapping the top-left instead of the center.

Comment 6

[Ann(Yiming)]

Attachment: small fix

Comment 7

[Malini Das [:mdas] - Away, not checking bugmail]

Comment on attachment 714493 [details] [diff] [review]
small fix

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

Cool, I just realized one more thing, and a style nit.

::: testing/marionette/client/marionette/www/testAction.html
@@ +35,5 @@
> +      link3.onmouseup = link3.innerHTML = "Clicked";
> +    }
> +  </script>
> +  <button id="mozLink" style="position:absolute;left:0px;top:55px;" type="button" onmousedown="press()" allowevents=true>Click Me!</button>
> +  <button id="mozLinkPos" style="position:absolute;left:0px;top:355px;" type="button" onmouseup="release()" allowevents=true>Position!</button>

Ah, I just noticed that we're using the shim here, and you're listening for mouse events. I'd like to reduce our reliance on the shim as much as possible (since it adds an extra layer of complexity that isn't needed for this test). Instead, you can remove the shim, and instead doing onmousedown/onmouseup, you can 'touchdown' and 'touchup' event listeners to mozLink and mozLinkPos respectively.

::: testing/marionette/marionette-listener.js
@@ +920,5 @@
> +      globalEle = el;
> +      lastX = corx;
> +      lastY = cory;
> +      emitTouchEvent('touchstart', touch);
> +      actions(finger,touchId, command_id, i+1);

why don't you just increment i before entering the switch? This style seems a little odd.

Comment 8

[Ann(Yiming)]

Attachment: removing shim file and style editing

Comment 9

[Malini Das [:mdas] - Away, not checking bugmail]

Comment on attachment 715733 [details] [diff] [review]
removing shim file and style editing

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

::: testing/marionette/client/marionette/www/testAction.html
@@ +39,5 @@
> +      press.innerHTML = "Clicked";
> +    }
> +    function changeReleaseText() {
> +      var release = document.getElementById("mozLinkPos");
> +      release.innerHTML = "Clicked";

This test case doesn't actually verify if the 'touchend' event was received on the 'mozLinkPos' element. All this does is make sure that 'touchend' is fired using 'mozLink' as its target. But that's not what we want to test here.

In changeReleaseText, you have to verify if the coordinates of the touch event received are within the bounding rect of 'mozLinkPos', and that will confirm if we dispatched the event to the right place on the page.

@@ +44,5 @@
> +    }
> +    function clicked() {
> +      var link2 = document.getElementById("scroll");
> +      link2.innerHTML = "Clicked";
> +    }

This function is never used. There are other unused things on this page. Please clean them up.

::: testing/marionette/marionette-listener.js
@@ +939,5 @@
> +      break;
> +    case 'move':
> +      el = elementManager.getKnownElement(pack[1], curWindow);
> +      touch = createATouch(el, null, null, touchId);
> +      globalEle = el;

https://developer.mozilla.org/en-US/docs/DOM/TouchEvent#touchmove like we talked about yesterday, you need to make *all* touch events relative to the first target, the target of touchstart.

Comment 10

[Ann(Yiming)]

Attachment: single finger + press/release

A couple of things to note here:
1. Press/Release functionality has been changed. Now press and release accept different coordinates with the target, i.e. id = element.press(0, 0), element.release(id, 1, 1) is valid with implicit touchmove. One test case has been created. Also, touchdown point and touchup point must be in the same viewport in order for the test to work.

2. shim.js file has been removed from the touch event testing (test_single_finger.py). In order to these tests to work, the action chain must happen within the same viewport also. 

3. Release in the action chain now does not consume any parameters because in order to release, user either needs to press or move to the point. Therefore, release happens wherever the finger is when "touchend" gets called.

Comment 11

[David Burns :automatedtester]

Comment on attachment 716943 [details] [diff] [review]
single finger + press/release

>--- /dev/null
>+++ b/testing/marionette/client/marionette/tests/unit/test_single_finger.py

>+        action = Actions(self.marionette)
>+        action.press(button).wait(5000).release()
>+        action.perform()


I am assuming that this is going to wait for 5000ms. Can we change the input to be in seconds so that it is more idiomatic with Python?

Comment 12

[Ann(Yiming)]

Attachment: Change into seconds

Changed into seconds. So now we can do wait(5).

Comment 13

[Malini Das [:mdas] - Away, not checking bugmail]

Comment on attachment 719117 [details] [diff] [review]
Change into seconds

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

This is awesome, thanks! There are just have a few changes I'd like to see.

::: testing/marionette/client/marionette/tests/unit/test_press_release.py
@@ +23,2 @@
>      def test_no_coordinates(self):
>        testTouch = self.marionette.absolute_url("testTouch.html")

Perhaps, see if you can replace testTouch.html with testAction.html completely. That would be nice!

::: testing/marionette/client/marionette/tests/unit/test_single_finger.py
@@ +7,5 @@
> +from marionette import Actions
> +
> +class testSingleFinger(MarionetteTestCase):
> +    def test_wait(self):
> +        testTouch = self.marionette.absolute_url("testTouch.html")

This is still using testTouch.html. can it use testAction.html so we remove the need for the shim?

::: testing/marionette/marionette-listener.js
@@ +242,5 @@
>    curWindow = content;
>    curWindow.focus();
>    touches = [];
>    touchIds = [];
> +  startEles = [];

Can you also set lastTouch to null here?

@@ +851,5 @@
>      var touchId = nextTouchId++;
>      var touch = createATouch(el, corx, cory, touchId);
>      emitTouchEvent('touchstart', touch);
>      touchIds.push(touchId);
> +    startEles.push(touch);

Instead of maintaining two arrays, do you think you can make touchIds an object similar to a dictionary, where the key is the touchId and the value is the startElement? This way we only maintain one object instead of two, and we don't need to do list operations like indexOf/splice, etc.

so we can do something like:
touchIds[touchId] = touch;

and replace the splice with delete commands:
delete touchIds[touchId]

so we can clear the memory.

@@ +889,5 @@
>        sendOk(msg.json.command_id);
>      }
>      else {
>        sendError("Element has not be pressed: InvalidElementCoordinates", 29, null, command_id);
>        return;

This is not part of your patch, but this return doesn't do anything. Can you remove it?

@@ +900,5 @@
>  
>  /**
> + * Function to emit touch events for each finger. e.g. pack=['press', id]
> + */
> +function actions(finger, touchId, command_id, i){

ah, I think it would be much nicer to check if i is defined, and if not, set it to 0, then increment it afterward. This way, we won't need to pass in -1 when we start the action chain. It shouldn't be a required parameter the first time you use it.

@@ +963,5 @@
> +    case 'wait':
> +      if (pack[1] != null ) {
> +        let time = pack[1]*1000;
> +        checkTimer.initWithCallback(function(){actions(finger, touchId, command_id, i);}, time, Ci.nsITimer.TYPE_ONE_SHOT);
> +        break;

you can move the breaks to out of the if/else blocks, and add it just at the end.

@@ +970,5 @@
> +        actions(finger, touchId, command_id, i);
> +        break;
> +      }
> +  }
> +  return;

Unnecessary return

Comment 14

[Ann(Yiming)]

Attachment: smallFixes

This patch replaced all "var" into "let"

Comment 15

[Malini Das [:mdas] - Away, not checking bugmail]

Comment on attachment 720167 [details] [diff] [review]
smallFixes

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

cool, just need a little bit more cleanup

::: testing/marionette/client/marionette/tests/unit/test_press_release.py
@@ +21,2 @@
>        self.marionette.navigate(testTouch)
> +      ele = self.marionette.find_element("id", "mozLinkHere")

I think you can use mozLinkCopy here and get rid of mozLinkHere

::: testing/marionette/client/marionette/www/testAction.html
@@ +16,5 @@
> +  <button id="mozLinkCopy" style="position:absolute;left:0px;top:455px;" type="button" allowevents=true>Button4</button>
> +  <button id="mozLinkStart" style="position:absolute;left:10px;top:200px;" type="button" allowevents=true>Press</button>
> +  <button id="mozLinkEnd" style="position:absolute;left:80px;top:200px;" type="button" allowevents=true>Release</button>
> +  <script type="text/javascript">
> +    window.ready = true;

Can you add some comments here about why we have to have so many elements? It's not obvious why we need this if the next reader of this test code doesn't know that the target of the touchstart event has to be referenced in all subsequent gestures until 'touchend'. It'll be helpful to clarify this, so we can help anyone who needs to look at this code and its related tests

@@ +29,5 @@
> +    second.addEventListener("touchend", changeClickText, false);
> +    third.addEventListener("touchstart", changeHorizontalPress, false);
> +    third.addEventListener("touchend", changeHorizontalRelease, false);
> +    fourth.addEventListener("touchstart", changeStartText, false);
> +    fourth.addEventListener("touchend", changeEndText, false);

why do we have both mozLinkCopy and mozLinkHere? It seems like mozLinkCopy can be used wherever mozLinkHere is being used in the tests

::: testing/marionette/marionette-listener.js
@@ +905,5 @@
> +    i = 0;
> +  }
> +  else {
> +    i++;
> +  }

you should remove this else statement and instead increment i after line 919, right before entering the switch

Comment 16

[Ann(Yiming)]

mozLinkHere is used for multi-finger touch. I will delete it here and add it in multi-finger patch.

Comment 17

[Ann(Yiming)]

Attachment: comments

Comment 18

[Malini Das [:mdas] - Away, not checking bugmail]

Comment on attachment 720992 [details] [diff] [review]
comments

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

Thanks, those comments make it a lot clearer

Comment 19

[Malini Das [:mdas] - Away, not checking bugmail]

Landed on inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7561dc42ece9

Comment 20

[Malini Das [:mdas] - Away, not checking bugmail]

Comment on attachment 720992 [details] [diff] [review]
comments

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

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: no direct user impact
Testing completed: 
Risk to taking this patch (and alternatives if risky): low
String or UUID changes made by this patch: none

I'd like to have this on b2g18 so that we can reach our goal of automating any gesture.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7561dc42ece9

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/6c546314d7cc

Comment 23

[Walter Chen[:ypwalter][:wachen]]

I think this is low risk to uplifting this into v1.0.0 and v1.0.1.
It would be good to have for gaia-ui-test to run modified test cases later!
Please help!

Comment 24

[Ryan VanderMeulen [:RyanVM]]

(In reply to Walter Chen from comment #23)
> I think this is low risk to uplifting this into v1.0.0 and v1.0.1.
> It would be good to have for gaia-ui-test to run modified test cases later!
> Please help!

v1.0.0 is closed. While this is technically test-only, I'm hesitant to land anything on v1.0.1 that isn't tef+/shira+. Please get approval from the B2G release drivers for v1.0.1 uplift.

Comment 25

[Walter Chen[:ypwalter][:wachen]]

Hi, who should I resort to for uplifting to v1.0.1?
It's low risk due to its nature of testing instead of something real in gecko.

Also, verified fixed in v1train 2013/03/11 pvt pub build

Comment 26

[Ryan VanderMeulen [:RyanVM]]

(In reply to Walter Chen from comment #25)
> Hi, who should I resort to for uplifting to v1.0.1?
> It's low risk due to its nature of testing instead of something real in
> gecko.
> 
> Also, verified fixed in v1train 2013/03/11 pvt pub build

Lukas?

Comment 28

[Jonathan Griffin (:jgriffin)]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/de94877b6ef5