Last Comment Bug 836375 - Add press() and release() tap gestures on elements
: Add press() and release() tap gestures on elements
Status: RESOLVED FIXED
:
Product: Testing
Classification: Components
Component: Marionette (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla21
Assigned To: Ann(Yiming)
:
Mentors:
Depends on:
Blocks: 750271
  Show dependency treegraph
 
Reported: 2013-01-30 10:12 PST by Malini Das [:mdas] - Away, not checking bugmail
Modified: 2013-05-01 14:40 PDT (History)
3 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
wontfix
wontfix
fixed
fixed
wontfix
fixed


Attachments
Adding press/release functionality (9.47 KB, patch)
2013-01-31 14:13 PST, Ann(Yiming)
no flags Details | Diff | Splinter Review
Adding coordinates (11.65 KB, patch)
2013-02-01 10:42 PST, Ann(Yiming)
no flags Details | Diff | Splinter Review
Changing tests to tap patch (10.21 KB, patch)
2013-02-01 11:33 PST, Ann(Yiming)
malini: review-
Details | Diff | Splinter Review
adding test (12.67 KB, patch)
2013-02-05 13:34 PST, Ann(Yiming)
no flags Details | Diff | Splinter Review
small changes (13.17 KB, patch)
2013-02-06 12:10 PST, Ann(Yiming)
malini: review+
lukasblakk+bugs: approval‑mozilla‑b2g18+
Details | Diff | Splinter Review

Description Malini Das [:mdas] - Away, not checking bugmail 2013-01-30 10:12:46 PST
Allow us to do something like:

element.press()
# do some checks
element.release()

press does a 'touchstart' and release does a 'touchend'

press()/release() should also take in coordinates. (ie: element.press(x,y)), where (x,y) are coordinates relative to the top-left of the element.
Comment 1 Ann(Yiming) 2013-01-31 14:13:07 PST
Created attachment 708795 [details] [diff] [review]
Adding press/release functionality
Comment 2 Malini Das [:mdas] - Away, not checking bugmail 2013-01-31 16:03:51 PST
Surprise, new requirements! :D

We'd like to be able to press/release on a coordinate relative to the current element. So

element.press(1,1) will press coordinate +1 on the x and +1 on the y axis from the top-left of the current element.
Comment 3 Jonathan Griffin (:jgriffin) 2013-01-31 18:07:42 PST
Yiming implemented this feature but found that press() worked while release() didn't.  I believe that may be due to:

https://developer.mozilla.org/en-US/docs/DOM/Touch.identifier
Returns a value uniquely identifying this point of contact with the touch surface. This value remains consistent for every event involving this finger's (or stylus's) movement on the surface until it is lifted off the surface.

In other words, both press() and release() will have to use the same touchId, which currently isn't the case.

How can we make them use the same id?  As an action chain it would be easy, since the action chain could set up the id.  When used without an action chain, the situation is a little murkier.  If release is only valid after press (plus potentially some intermediate actions like move), we could have press() set a touchId which would get used by all subsequent methods until release() is called.

I'm not sure how this would work with multi-finger touches, but we'll probably need a more complex interface for that anyway, and that interface could set up a unique touch id for each finger.
Comment 4 Ann(Yiming) 2013-01-31 18:16:52 PST
hmm it is possible to use the same id if I use a queue to keep track of the ids. But I feel like that's what action chain going to do. Is my understanding correct?
Comment 5 Jonathan Griffin (:jgriffin) 2013-01-31 18:23:00 PST
One idea is to have press() return a touchId, and to have other methods consume it:

touchId = element.press()
element.release(touchId)

This would allow multi-finger gestures to be utilized in a test:

finger1 = element1.press()
finger2 = element2.press()
element2.release(finger2)
element1.release(finger1)
Comment 6 Malini Das [:mdas] - Away, not checking bugmail 2013-02-01 05:56:56 PST
(In reply to Jonathan Griffin (:jgriffin) from comment #5)
> One idea is to have press() return a touchId, and to have other methods
> consume it:
> 
> touchId = element.press()
> element.release(touchId)
> 
> This would allow multi-finger gestures to be utilized in a test:
> 
> finger1 = element1.press()
> finger2 = element2.press()
> element2.release(finger2)
> element1.release(finger1)

Ah yeah, that's a good solution, I think we should go forward with this
Comment 7 Ann(Yiming) 2013-02-01 08:20:29 PST
(In reply to Malini Das [:mdas] from comment #6)
> (In reply to Jonathan Griffin (:jgriffin) from comment #5)
> > One idea is to have press() return a touchId, and to have other methods
> > consume it:
> > 
> > touchId = element.press()
> > element.release(touchId)
> > 
> > This would allow multi-finger gestures to be utilized in a test:
> > 
> > finger1 = element1.press()
> > finger2 = element2.press()
> > element2.release(finger2)
> > element1.release(finger1)
> 
> Ah yeah, that's a good solution, I think we should go forward with this

So instead of taking in coordinates for release(), we want pass in TouchId to release instead?
Comment 8 Jonathan Griffin (:jgriffin) 2013-02-01 10:00:13 PST
(In reply to Ann(Yiming) from comment #7)
> (In reply to Malini Das [:mdas] from comment #6)
> > (In reply to Jonathan Griffin (:jgriffin) from comment #5)
> > > One idea is to have press() return a touchId, and to have other methods
> > > consume it:
> > > 
> > > touchId = element.press()
> > > element.release(touchId)
> > > 
> > > This would allow multi-finger gestures to be utilized in a test:
> > > 
> > > finger1 = element1.press()
> > > finger2 = element2.press()
> > > element2.release(finger2)
> > > element1.release(finger1)
> > 
> > Ah yeah, that's a good solution, I think we should go forward with this
> 
> So instead of taking in coordinates for release(), we want pass in TouchId
> to release instead?

No, we'll take both:

element1.release(touchId, x, y).  x,y can be optional, but touchId should be mandatory.
Comment 9 Ann(Yiming) 2013-02-01 10:08:41 PST
(In reply to Jonathan Griffin (:jgriffin) from comment #8)
> (In reply to Ann(Yiming) from comment #7)
> > (In reply to Malini Das [:mdas] from comment #6)
> > > (In reply to Jonathan Griffin (:jgriffin) from comment #5)
> > > > One idea is to have press() return a touchId, and to have other methods
> > > > consume it:
> > > > 
> > > > touchId = element.press()
> > > > element.release(touchId)
> > > > 
> > > > This would allow multi-finger gestures to be utilized in a test:
> > > > 
> > > > finger1 = element1.press()
> > > > finger2 = element2.press()
> > > > element2.release(finger2)
> > > > element1.release(finger1)
> > > 
> > > Ah yeah, that's a good solution, I think we should go forward with this
> > 
> > So instead of taking in coordinates for release(), we want pass in TouchId
> > to release instead?
> 
> No, we'll take both:
> 
> element1.release(touchId, x, y).  x,y can be optional, but touchId should be
> mandatory.

Thanks for clarifying it. That is how I implemented it! :)
Comment 10 Ann(Yiming) 2013-02-01 10:42:10 PST
Created attachment 709130 [details] [diff] [review]
Adding coordinates
Comment 11 Ann(Yiming) 2013-02-01 11:33:14 PST
Created attachment 709164 [details] [diff] [review]
Changing tests to tap patch
Comment 12 Malini Das [:mdas] - Away, not checking bugmail 2013-02-04 06:16:42 PST
Comment on attachment 709164 [details] [diff] [review]
Changing tests to tap patch

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

Oh also, please add tests to the patch.

::: testing/marionette/marionette-listener.js
@@ +58,5 @@
>  // The current array of all pending touches
>  let touches = [];
>  // For assigning unique ids to all touches
>  let nextTouchId = 1000;
> +let touchStartBool = false;

This boolean doesn't really work with the idea of multiple press()/release() calls. It will allow us to call release for the same element twice. For example:

id1 = element1.press()
id2 = element2.press()
release(id1)
release(id2)

Will be allowed with this boolean design. Instead, we should keep an array of all ids seen and remove them when the release() is called for that id.

@@ +796,5 @@
>  
>  /**
> + * Function to create a touch based on the element
> + */
> +function createTouches(el, corx, cory, id) {

Can we change the name of this function to "createTouch" since it only creates one Touch object?
Comment 13 Ann(Yiming) 2013-02-05 13:34:52 PST
Created attachment 710361 [details] [diff] [review]
adding test
Comment 14 Malini Das [:mdas] - Away, not checking bugmail 2013-02-06 11:15:42 PST
Comment on attachment 710361 [details] [diff] [review]
adding test

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

Great, just a few nits to fix.

One more thing I just noticed. The touches and touch_ids arrays should be cleared when deleteSession is called in marionette-listener.js. So in the deleteSession function, just set these arrays to []. That's because if we ever delete a session then start a new one, we don't want to maintain a list of the previous session's touch events. Can you add that in?

::: testing/marionette/client/marionette/tests/unit/test_press_release.py
@@ +4,5 @@
> +
> +import os
> +import time
> +from marionette_test import MarionetteTestCase
> +from marionette import HTMLElement

HTMLElement isn't being used here, you should remove this import

::: testing/marionette/marionette-listener.js
@@ +59,5 @@
>  // The current array of all pending touches
>  let touches = [];
>  // For assigning unique ids to all touches
>  let nextTouchId = 1000;
> +let touch_ids = [];

should be touchIds, since we use camel-case in JS
Comment 15 Ann(Yiming) 2013-02-06 12:10:23 PST
Created attachment 710873 [details] [diff] [review]
small changes
Comment 16 Malini Das [:mdas] - Away, not checking bugmail 2013-02-06 12:43:05 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/b3e63d9ea7ac
Comment 17 Ryan VanderMeulen [:RyanVM] 2013-02-06 18:08:01 PST
https://hg.mozilla.org/mozilla-central/rev/b3e63d9ea7ac
Comment 18 Malini Das [:mdas] - Away, not checking bugmail 2013-02-12 10:01:27 PST
This will be very helpful for testing against mozilla-b2g18. It only affects the Marionette test framework.
Comment 19 Malini Das [:mdas] - Away, not checking bugmail 2013-02-13 13:19:20 PST
Comment on attachment 710873 [details] [diff] [review]
small changes

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

This will be very helpful for testing against mozilla-b2g18. It only affects the Marionette test framework.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: 
Testing completed: 
Risk to taking this patch (and alternatives if risky): 
This removes some extra logging accidentally left from a previous patch. 
Test-only and no user impact.
String or UUID changes made by this patch:
Comment 20 Lukas Blakk [:lsblakk] use ?needinfo 2013-02-20 08:27:33 PST
Comment on attachment 710873 [details] [diff] [review]
small changes

Approving for uplift to v1-train, marionette only.
Comment 21 Malini Das [:mdas] - Away, not checking bugmail 2013-02-20 12:36:43 PST
landed in mozilla-b2g18:
https://hg.mozilla.org/releases/mozilla-b2g18/rev/9902641dcac5
Comment 22 Ryan VanderMeulen [:RyanVM] 2013-02-21 02:55:52 PST
Backed out for turning B2G Marionette perma-orange.
https://hg.mozilla.org/releases/mozilla-b2g18/rev/b29d929fa237

https://tbpl.mozilla.org/php/getParsedLog.php?id=19925217&tree=Mozilla-B2g18

14:07:23     INFO -  TEST-START test_press_release.py
14:07:36     INFO -  test_coordinates (test_press_release.testPressRelease) ... /home/cltbld/talos-slave/test/build/tests/marionette/marionette/errors.py:15: DeprecationWarning: BaseException.message has been deprecated as of Python 2.6
14:07:36     INFO -    return '%s\nstacktrace:\n%s' % (str(self.message),
14:07:36     INFO -  ERROR
14:07:48     INFO -  test_no_coordinates (test_press_release.testPressRelease) ... ok
14:07:48     INFO -  ======================================================================
14:07:48     INFO -  ERROR: test_coordinates (test_press_release.testPressRelease)
14:07:48     INFO -  ----------------------------------------------------------------------
14:07:48    ERROR -  Traceback (most recent call last):
14:07:48     INFO -    File "/home/cltbld/talos-slave/test/build/tests/marionette/tests/testing/marionette/client/marionette/tests/unit/test_press_release.py", line 17, in test_coordinates
14:07:48     INFO -      self.assertEqual("Clicked", self.marionette.execute_script("return document.getElementById('mozLinkPos').innerHTML;"))
14:07:48     INFO -    File "/home/cltbld/talos-slave/test/build/tests/marionette/marionette/marionette.py", line 479, in execute_script
14:07:48     INFO -      specialPowers=special_powers)
14:07:48     INFO -    File "/home/cltbld/talos-slave/test/build/tests/marionette/marionette/marionette.py", line 239, in _send_message
14:07:48     INFO -      self._handle_error(response)
14:07:48     INFO -    File "/home/cltbld/talos-slave/test/build/tests/marionette/marionette/marionette.py", line 274, in _handle_error
14:07:48    ERROR -      raise JavascriptException(message=message, status=status, stacktrace=stacktrace)
14:07:48    ERROR -  JavascriptException: TypeError: document.getElementById(...) is null
14:07:48     INFO -  stacktrace:
14:07:48     INFO -  	__marionetteFunc@chrome://marionette/content/marionette-listener.js:429
14:07:48     INFO -  TEST-UNEXPECTED-FAIL | test_press_release.py testPressRelease.test_coordinates | 	@chrome://marionette/content/marionette-listener.js:429
14:07:48     INFO -  ----------------------------------------------------------------------
14:07:48     INFO -  Ran 2 tests in 25.511s
14:07:48    ERROR -  FAILED (errors=1)
Comment 23 Malini Das [:mdas] - Away, not checking bugmail 2013-02-21 09:45:25 PST
Thanks, it seems Bug 836952 has to land in mozilla-b2g18 first before this patch can safely land.
Comment 24 Ryan VanderMeulen [:RyanVM] 2013-02-22 10:20:58 PST
https://hg.mozilla.org/releases/mozilla-b2g18/rev/01e9fecb5187
Comment 25 Jonathan Griffin (:jgriffin) 2013-05-01 14:40:07 PDT
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/608bd5a5c21e

Note You need to log in before you can comment on or make changes to this bug.