836375 - Add press() and release() tap gestures on elements

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect)

Description

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

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

Attachment: Adding press/release functionality

Comment 2

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

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

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

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

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]

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

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

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

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

Attachment: Adding coordinates

Comment 11

[Ann(Yiming)]

Attachment: Changing tests to tap patch

Comment 12

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

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

Attachment: adding test

Comment 14

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

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

Attachment: small changes

Comment 16

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/b3e63d9ea7ac

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b3e63d9ea7ac

Comment 18

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

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]

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]

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]

landed in mozilla-b2g18:
https://hg.mozilla.org/releases/mozilla-b2g18/rev/9902641dcac5

Comment 22

[Ryan VanderMeulen [:RyanVM]]

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]

Thanks, it seems Bug 836952 has to land in mozilla-b2g18 first before this patch can safely land.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/01e9fecb5187

Comment 25

[Jonathan Griffin (:jgriffin)]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/608bd5a5c21e