The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in Firefox 21

Status

Testing
Marionette
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: mdas, Assigned: Ann(Yiming))

Tracking

Trunk
mozilla21
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox19 wontfix, firefox20 wontfix, firefox21 fixed, b2g18 fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 fixed)

Details

Attachments

(1 attachment, 4 obsolete attachments)

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.
OS: Mac OS X → All
Hardware: x86 → All
(Assignee)

Comment 1

4 years ago
Created attachment 708795 [details] [diff] [review]
Adding press/release functionality
Attachment #708795 - Flags: review?(mdas)
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.
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.
(Assignee)

Comment 4

4 years ago
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?
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)
(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
(Assignee)

Comment 7

4 years ago
(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?
(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.
(Assignee)

Comment 9

4 years ago
(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! :)
(Assignee)

Comment 10

4 years ago
Created attachment 709130 [details] [diff] [review]
Adding coordinates
Attachment #708795 - Attachment is obsolete: true
Attachment #708795 - Flags: review?(mdas)
Attachment #709130 - Flags: review?(mdas)
(Assignee)

Comment 11

4 years ago
Created attachment 709164 [details] [diff] [review]
Changing tests to tap patch
Attachment #709130 - Attachment is obsolete: true
Attachment #709130 - Flags: review?(mdas)
Attachment #709164 - Flags: review?(mdas)
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?
Attachment #709164 - Flags: review?(mdas) → review-
(Assignee)

Comment 13

4 years ago
Created attachment 710361 [details] [diff] [review]
adding test
Attachment #709164 - Attachment is obsolete: true
Attachment #710361 - Flags: review?(mdas)
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
(Assignee)

Comment 15

4 years ago
Created attachment 710873 [details] [diff] [review]
small changes
Attachment #710361 - Attachment is obsolete: true
Attachment #710361 - Flags: review?(mdas)
Attachment #710873 - Flags: review?(mdas)
Attachment #710873 - Flags: review?(mdas) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/b3e63d9ea7ac
https://hg.mozilla.org/mozilla-central/rev/b3e63d9ea7ac
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
This will be very helpful for testing against mozilla-b2g18. It only affects the Marionette test framework.
tracking-b2g18: --- → ?
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:
Attachment #710873 - Flags: approval-mozilla-b2g18?
tracking-b2g18: ? → ---
status-b2g18: --- → affected
status-b2g18-v1.0.0: --- → wontfix
status-b2g18-v1.0.1: --- → wontfix
Comment on attachment 710873 [details] [diff] [review]
small changes

Approving for uplift to v1-train, marionette only.
Attachment #710873 - Flags: approval-mozilla-b2g18? → approval-mozilla-b2g18+
landed in mozilla-b2g18:
https://hg.mozilla.org/releases/mozilla-b2g18/rev/9902641dcac5
status-b2g18: affected → fixed
status-firefox19: --- → wontfix
status-firefox20: --- → wontfix
status-firefox21: --- → fixed
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)
status-b2g18: fixed → affected
Thanks, it seems Bug 836952 has to land in mozilla-b2g18 first before this patch can safely land.
https://hg.mozilla.org/releases/mozilla-b2g18/rev/01e9fecb5187
status-b2g18: affected → fixed
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/608bd5a5c21e
status-b2g18-v1.0.1: wontfix → fixed
You need to log in before you can comment on or make changes to this bug.