Closed Bug 841101 Opened 7 years ago Closed 7 years ago

Add support for multi touch action chains in marionette

Categories

(Testing :: Marionette, defect)

defect
Not set

Tracking

(firefox22 fixed, b2g18 fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 fixed)

RESOLVED FIXED
mozilla22
Tracking Status
firefox22 --- fixed
b2g18 --- fixed
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- fixed

People

(Reporter: mdas, Assigned: annyang121)

Details

Attachments

(1 file, 9 obsolete files)

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
Attached patch multiTouch patch (obsolete) — Splinter Review
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.
Attachment #719128 - Flags: review?(mdas)
Attached patch styleFix (obsolete) — Splinter Review
Style fix:
1. Changed all var into let
2. Added comment
3. Changed the way accumulator gets called
Attachment #719128 - Attachment is obsolete: true
Attachment #719128 - Flags: review?(mdas)
Attachment #720912 - Flags: review?(mdas)
Attached patch moreStyleFix (obsolete) — Splinter Review
Attachment #720912 - Attachment is obsolete: true
Attachment #720912 - Flags: review?(mdas)
Attachment #721004 - Flags: review?(mdas)
Attached patch checkVisible (obsolete) — Splinter Review
Attachment #721004 - Attachment is obsolete: true
Attachment #721004 - Flags: review?(mdas)
Attachment #722953 - Flags: review?(mdas)
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
Attachment #722953 - Flags: review?(mdas) → review-
Attached patch multiTouch (obsolete) — Splinter Review
Attachment #722953 - Attachment is obsolete: true
Attachment #724588 - Flags: review?(mdas)
Attached patch styleFix for testAction.html (obsolete) — Splinter Review
changePressText now accepts a string so that we won't have duplicate functions
Attachment #724588 - Attachment is obsolete: true
Attachment #724588 - Flags: review?(mdas)
Attachment #724604 - Flags: review?(mdas)
Attached patch smallFixes (obsolete) — Splinter Review
Attachment #724604 - Attachment is obsolete: true
Attachment #724604 - Flags: review?(mdas)
Attachment #724623 - Flags: review?(mdas)
Attached patch htmlFix (obsolete) — Splinter Review
Attachment #724623 - Attachment is obsolete: true
Attachment #724623 - Flags: review?(mdas)
Attachment #725020 - Flags: review?(mdas)
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.
Attachment #725020 - Flags: review?(mdas) → review-
oh, one more thing! can you up the python package version too? I remembered before r+ing this time:)
Will do! Do we want to up the python package version in all future patches?
(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!
Attached patch htmlFix+upVersion (obsolete) — Splinter Review
Attachment #725020 - Attachment is obsolete: true
Attachment #726293 - Flags: review?(mdas)
Attached patch addToInitSplinter Review
Attachment #726293 - Attachment is obsolete: true
Attachment #726293 - Flags: review?(mdas)
Attachment #726320 - Flags: review?(mdas)
Comment on attachment 726320 [details] [diff] [review]
addToInit

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

Great! I'll land asap
Attachment #726320 - Flags: review?(mdas) → review+
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.
https://hg.mozilla.org/mozilla-central/rev/284df6d78bc5
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
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.
Attachment #726320 - Flags: approval-mozilla-b2g18?
Attachment #726320 - Flags: approval-mozilla-b2g18? → approval-mozilla-b2g18+
This doesn't apply cleanly to b2g18. Looks like it's a few versions behind in Marionette versions.
You need to log in before you can comment on or make changes to this bug.