841101 - Add support for multi touch action chains in marionette

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect)

Description

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

Once Bug 839943 lands, we'll have action chains for a single finger. We also want to be able to automate a multifinger gesture. 

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()

This is to track the implementation of MultiActions

Comment 1

[Ann(Yiming)]

Attachment: multiTouch patch

Notes:
1. All fingers are required to perform within the same viewport.
2. Since we do not require each finger to be on some element, visibility of the elements are not checked.

Comment 2

[Ann(Yiming)]

Attachment: styleFix

Style fix:
1. Changed all var into let
2. Added comment
3. Changed the way accumulator gets called

Comment 3

[Ann(Yiming)]

Attachment: moreStyleFix

Comment 4

[Ann(Yiming)]

Attachment: checkVisible

Comment 5

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

Comment on attachment 722953 [details] [diff] [review]
checkVisible

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

Great to see this working!

::: testing/marionette/client/marionette/tests/unit/test_multi_finger.py
@@ +54,5 @@
> +      action1.press(start_one).move_by_offset(0,300).wait().release()
> +      action2.press(ele).wait(5).release()
> +      action3.press(start_two).wait(3).move(drop_two).wait(2).release()
> +      multi_action.add(action1).add(action2).add(action3).perform()
> +      time.sleep(15)

These tests verify that all the actions were completed, but doesn't test if they fired off at the right times, and we want to make sure that order and timing is preserved.

You can use event listener functions to either replace the html or update a variable with the current time when an event was fired, so you can compare when the actions were dispatched. I'm mostly concerned with making sure that the wait() with no parameters actually does wait its trun and that wait(3) waits at least 3 seconds between move and release. 

We should make sure that action2's release() is called at least 5 seconds after press was called, and test that action3's move(drop_two) is called at least 5 seconds (since it's waiting on action 2) after press() was called. Any other tests like this are welcome. We should take steps to verify that the action chains are fired off in the right order/timing.

Feel free to change the events and their orders so that you can make easier measurements, or add a new test for this if you prefer.

::: testing/marionette/marionette-listener.js
@@ +1030,5 @@
> +}
> +
> +/**
> + * Function to dispatch one set of actions
> + * @param touches represents all pending touches, patchIndex represents the patch we are dispatching right now

can patch be replaced by the word 'batch' in this comment and in the rest of the code? it makes more sense in this context, since 'patch' is used to denote a fix in code, whereas 'batch' is for a group of things

@@ +1131,5 @@
> +        break;
> +    }//end of switch block
> +  }//end of for loop
> +  if (maxTime != 0) {
> +    checkTimer.initWithCallback(function(){setDispatch(patches, touches, command_id, patchIndex);}, maxTime, Ci.nsITimer.TYPE_ONE_SHOT);

This function will return before all actions are completed. If you have 3 batches, and the 2nd batch has a wait, then this if statement will be reached (so the timer is set), and then setDispatch will implicitly return control back to line 1171, where sendOk() will be sent prematurely; it will be sent before batch 3 will be completed :) You'll need to revisit how you sendOk back to the user in this case

Comment 6

[Ann(Yiming)]

Attachment: multiTouch

Comment 7

[Ann(Yiming)]

Attachment: styleFix for testAction.html

changePressText now accepts a string so that we won't have duplicate functions

Comment 8

[Ann(Yiming)]

Attachment: smallFixes

Comment 9

[Ann(Yiming)]

Attachment: htmlFix

Comment 10

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

Comment on attachment 725020 [details] [diff] [review]
htmlFix

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

Great, thanks, I just tested it out and it works. There's just one more coding nit, and then we can land it.

::: testing/marionette/client/marionette/www/testAction.html
@@ +86,5 @@
> +          clientX <= boxr.right) {
> +        var d = new Date();
> +        var timeDiff = d.getTime() - press.innerHTML;
> +        release.innerHTML = timeDiff;
> +      }

again, can this be put into a helper function? Repetitive code makes the script difficult to read since the meaningful, unique parts of the code are hard to find. 

A similar block of code is being used in changeReleaseText, and you can just rip out the part that checks if the touch event is triggered on the right element as a function, and have that function return a boolean. You can then check the boolean and do the innerHTML stuff.

Comment 11

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

oh, one more thing! can you up the python package version too? I remembered before r+ing this time:)

Comment 12

[Ann(Yiming)]

Will do! Do we want to up the python package version in all future patches?

Comment 13

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

(In reply to Ann(Yiming) from comment #12)
> Will do! Do we want to up the python package version in all future patches?

Only when the marionette client's python files are changed, thanks!

Comment 14

[Ann(Yiming)]

Attachment: htmlFix+upVersion

Comment 15

[Ann(Yiming)]

Attachment: addToInit

Comment 16

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

Comment on attachment 726320 [details] [diff] [review]
addToInit

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

Great! I'll land asap

Comment 17

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

landed on m-i:
https://hg.mozilla.org/integration/mozilla-inbound/rev/284df6d78bc5

once it lands on m-c and is green, I'll ask for b2g18 landing approval.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/284df6d78bc5

Comment 19

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

Comment on attachment 726320 [details] [diff] [review]
addToInit

Review of attachment 726320 [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 20

[Ryan VanderMeulen [:RyanVM]]

This doesn't apply cleanly to b2g18. Looks like it's a few versions behind in Marionette versions.

Comment 21

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

https://hg.mozilla.org/releases/mozilla-b2g18/pushloghtml?changeset=71f7f42dee13

Comment 22

[Jonathan Griffin (:jgriffin)]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/9332dc05bd1d