Last Comment Bug 833448 - add singleTap and doubleTap ability to marionette
: add singleTap and doubleTap ability to marionette
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: 845291
Blocks: 750271
  Show dependency treegraph
 
Reported: 2013-01-22 10:37 PST by Malini Das [:mdas] - Away, not checking bugmail
Modified: 2013-05-01 14:41 PDT (History)
4 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
wontfix
wontfix
fixed
fixed
wontfix
fixed


Attachments
added singleTap and doubleTap functionality (21.55 KB, patch)
2013-01-22 11:36 PST, Ann(Yiming)
no flags Details | Diff | Splinter Review
Fixing conflicts (14.50 KB, patch)
2013-01-22 15:04 PST, Ann(Yiming)
no flags Details | Diff | Splinter Review
added exceptions (15.70 KB, patch)
2013-01-28 10:32 PST, Ann(Yiming)
no flags Details | Diff | Splinter Review
added exceptions with new test cases (27.84 KB, patch)
2013-01-28 10:44 PST, Ann(Yiming)
no flags Details | Diff | Splinter Review
Small changes regarding variable names and error messages (27.86 KB, patch)
2013-01-29 09:47 PST, Ann(Yiming)
no flags Details | Diff | Splinter Review
Using HTMLElement (27.33 KB, patch)
2013-01-29 10:56 PST, Ann(Yiming)
malini: review-
Details | Diff | Splinter Review
smallFixes (30.00 KB, patch)
2013-01-30 17:16 PST, Ann(Yiming)
no flags Details | Diff | Splinter Review
One more formatting fix (30.04 KB, patch)
2013-01-31 08:18 PST, Ann(Yiming)
malini: review-
Details | Diff | Splinter Review
checkVisible (29.98 KB, patch)
2013-01-31 09:44 PST, Ann(Yiming)
malini: review+
lukasblakk+bugs: approval‑mozilla‑b2g18+
Details | Diff | Splinter Review

Description Malini Das [:mdas] - Away, not checking bugmail 2013-01-22 10:37:29 PST
As in selenium: http://code.google.com/searchframe#2tHw6m3DZzo/trunk/java/client/src/org/openqa/selenium/interactions/touch/TouchActions.java&q=touchaction%20package:selenium\.googlecode\.com&sq=&ct=rc&cd=3)

and in our python abstraction layer, we want to implement singleTap and doubleTap, but as part of the marionette server
Comment 1 Ann(Yiming) 2013-01-22 11:36:23 PST
Created attachment 705026 [details] [diff] [review]
added singleTap and doubleTap functionality
Comment 2 David Burns :automatedtester 2013-01-22 12:19:18 PST
can this patch be rebased since it removes functionality required for Bug 814768
Comment 3 Ann(Yiming) 2013-01-22 15:04:14 PST
Created attachment 705128 [details] [diff] [review]
Fixing conflicts
Comment 4 Malini Das [:mdas] - Away, not checking bugmail 2013-01-23 10:09:04 PST
Comment on attachment 705128 [details] [diff] [review]
Fixing conflicts

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

Please mark older versions of your patches as 'obsolete' when you attach a new patch, so we don't have old patches lying around :)

So we're going to delay the review of this patch for now. We're going to test out using sendTouchEvent instead of synthesizing all the events ourselves. In addition, I'd like to add some exception throwing in the event that the element we want to tap is not visible, or if it can't be scrolled to.

For visibility, once you get the element (el = elementManager.getKnownElement(msg.json.value, curWindow);) you can then do what the isElementDisplayed function does in marionette-listener.js and call utils.isElementDisplayed on it. if it returns false, you should send an error matching ElementNotVisible (http://code.google.com/p/selenium/wiki/JsonWireProtocol#Response_Status_Codes)

For the scrolling, the idea is that we IDEALLY want to tap an element that is currently in the viewport. It is not necessary (https://dvcs.w3.org/hg/webdriver/raw-file/tip/webdriver-spec.html#clicking, it says "the implementation should bring the element into view first."). Essentially, if the element can be scrolled to, then we want to scroll to it and then tap it. If we try to scroll to it and fail, then we want to throw this error. You can scroll an element into view by getting the element with the element manager, then calling scrollIntoView() on it (https://developer.mozilla.org/en-US/docs/DOM/element.scrollIntoView). The problem is that not all elements have this function, so you have to first check if that function is available on the element, and if so, call it. If scrollIntoView is not available, then it's safe to ignore it for now. The problem we have here is: if we can call scrollIntoView, and we call it, how do we determine that it was successfully scrolled and is now in the viewport? That's an interesting issue to solve. I think you can do it by checking the element's position offset against the window's page offset. I think some googling here will help you.

::: testing/marionette/client/marionette/marionette.py
@@ +394,5 @@
>      def timeouts(self, timeout_type, ms):
>          assert(timeout_type == self.TIMEOUT_SEARCH or timeout_type == self.TIMEOUT_SCRIPT or timeout_type == self.TIMEOUT_PAGE)
>          response = self._send_message('timeouts', 'ok', timeoutType=timeout_type, ms=ms)
>          return response
> + 

A spare bit of whitespace here

::: testing/marionette/marionette-listener.js
@@ +683,5 @@
> +  if (tx0 === 'number')
> +    coords.x0 = box.left + x0;
> +  else if (tx0 === 'string')
> +    coords.x0 = box.left + percent(x0, box.width);
> +  

there are some extra white spaces in this function. If you look at the 'Splinter Review' view of this attachment and click on 'All files' you'll see the extra whitespace highlighted in red.
Comment 5 Malini Das [:mdas] - Away, not checking bugmail 2013-01-23 10:09:32 PST
Comment on attachment 705026 [details] [diff] [review]
added singleTap and doubleTap functionality

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

this is an obsolete patch.
Comment 6 Ann(Yiming) 2013-01-28 10:32:28 PST
Created attachment 707179 [details] [diff] [review]
added exceptions

After the tap event is fired, we need to wait for 10 seconds in order to see the result of tapping.
Comment 7 Ann(Yiming) 2013-01-28 10:44:46 PST
Created attachment 707186 [details] [diff] [review]
added exceptions with new test cases
Comment 8 David Burns :automatedtester 2013-01-29 06:52:04 PST
Comment on attachment 707179 [details] [diff] [review]
added exceptions

From a quick look, this looks great but I have a few things I would like to note:

Adding doubleTap and singleTap to Marionette object is creating a God Object[1] which kind of defeats the purposes of OO.

We split these off into touch actions[2] in selenium to keep the "Driver" object as succinct as possible.

Other thing to note is the python methods are not idiomatic, would prefer single_tap and double_tap.

[1] http://en.wikipedia.org/wiki/God_object
[2] https://code.google.com/p/selenium/source/browse/py/selenium/webdriver/common/touch_actions.py


Brilliant work!!!




>+ 
>+    def singleTap(self, element):
>+        element = element.id
>+        response = self._send_message('singleTap', 'ok', element=element)
>+        return response
>+
>+    def doubleTap(self, element):
>+        element = element.id
>+        response = self._send_message('doubleTap', 'ok', element=element)
>+        return response
> 



>+    let bool = utils.isElementDisplayed(el);
>+    if (!bool){

This is just my preference but I think the variable name of visible would be better than bool. That way on the next line if (!visible) { reads much nicer


>+    if (!success){
>+      sendError("The command not executed successfully.", 0, null, command_id);


I would send an element not visible and if we are going to say a command wasn't executed successfully. 
http://mxr.mozilla.org/mozilla-central/source/testing/marionette/client/marionette/errors.py#77


>+      return;
>+    }
>+    // let doc = curWindow.document;
I guess it's not needed :)



>+    el = elementManager.getKnownElement(msg.json.value, curWindow);
>+    let bool = utils.isElementDisplayed(el);

This is just my preference but I think the variable name of visible would be better than bool. That way on the next line if (!visible) { reads much nicer

>+    if (!bool){



>+      sendError("The command not executed successfully.", 0, null, command_id);

I would send an element not visible and if we are going to say a command wasn't executed successfully. 
http://mxr.mozilla.org/mozilla-central/source/testing/marionette/client/marionette/errors.py#77
Comment 9 Malini Das [:mdas] - Away, not checking bugmail 2013-01-29 07:32:24 PST
(In reply to David Burns :automatedtester from comment #8)
> Comment on attachment 707179 [details] [diff] [review]
> added exceptions
> 
> From a quick look, this looks great but I have a few things I would like to
> note:
> 
> Adding doubleTap and singleTap to Marionette object is creating a God
> Object[1] which kind of defeats the purposes of OO.
> 
> We split these off into touch actions[2] in selenium to keep the "Driver"
> object as succinct as possible.
> 
> Other thing to note is the python methods are not idiomatic, would prefer
> single_tap and double_tap.
> 
> [1] http://en.wikipedia.org/wiki/God_object
> [2]
> https://code.google.com/p/selenium/source/browse/py/selenium/webdriver/
> common/touch_actions.py
> 
> 
> Brilliant work!!!
> 
Interesting point. This will be helpful for future test cases.

To elaborate, we want to add single_tap and double_tap to the HTMLElement instead of the Marionette object. The reasoning behind this is that we want to eventually be able to batch a bunch of actions on elements together and perform them at the same time, and tap is one of those actions. What AutomatedTester would like to see is something like this:

actions = ActionBuilder(self.marionette)
actions.single_tap(element).touchdown(element).touchmove(<some parameters>).touchup(<some parameters>)
build = actions.build()
build.perform()

Which will let us single_tap and element and then drag and drop it somewhere. 

This model is helpful for test-writers because then, they don't need to keep writing a long list of actions to do a drag and drop, they can just use the ActionBuilder to create a drag and drop action once, then keep calling perform on it to do it again.

The ActionBuilder will use small actions we can perform to build larger actions. So AutomatedTester is suggesting we add single_tap and double_tap as part of these small actions. To do that, instead of adding it to the Marionette object, add the single_tap and double_tap functions as part of the HTMLElement. You'll have to modify how you tap the elements by doing:

element.single_tap()

instead of self.marionette.single_tap(element).

Don't worry about the ActionBuilder stuff yet. We can add that as part of a following patch.
Comment 10 Ann(Yiming) 2013-01-29 09:47:53 PST
Created attachment 707702 [details] [diff] [review]
Small changes regarding variable names and error messages
Comment 11 Ann(Yiming) 2013-01-29 10:56:40 PST
Created attachment 707730 [details] [diff] [review]
Using HTMLElement
Comment 12 Malini Das [:mdas] - Away, not checking bugmail 2013-01-30 10:08:38 PST
Comment on attachment 707730 [details] [diff] [review]
Using HTMLElement

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

Good start!

You have to add the shim.js file to the patch as well.

::: testing/marionette/client/marionette/marionette.py
@@ +48,5 @@
>          return self.marionette._send_message('getElementAttribute', 'value', element=self.id, name=attribute)
>  
>      def click(self):
>          return self.marionette._send_message('clickElement', 'ok', element=self.id)
> +    

extra whitespace

@@ +52,5 @@
> +    
> +    def single_tap(self):
> +        response = self.marionette._send_message('singleTap', 'ok', element=self.id)
> +        return response
> +    

same here

@@ +55,5 @@
> +        return response
> +    
> +    def double_tap(self):
> +        response = self.marionette._send_message('doubleTap', 'ok', element=self.id)
> +        return response

can you match the same style as the other functions in this class? ie: 

return self.marionette._send_message('doubleTap', 'ok', element=self.id) 

in this case

::: testing/marionette/client/marionette/tests/unit/test_touch.py
@@ +5,5 @@
> +import os
> +import time
> +from marionette_test import MarionetteTestCase
> +from marionette import HTMLElement
> +from errors import NoSuchElementException, JavascriptException, MarionetteException, ScriptTimeoutException

None of these errors are used, so you should remove this line

@@ +23,5 @@
> +	ele = self.marionette.find_element("id", "testh2")
> +      	self.assertRaises(MarionetteException, ele.single_tap)
> +	
> +    def test_scrolling(self):
> +	testTouch = self.marionette.absolute_url("testTouch.html")	

extra whitespace at the end of this sentence

@@ +28,5 @@
> + 	self.marionette.navigate(testTouch)
> +	ele = self.marionette.find_element("id", "scroll")
> +	ele.single_tap()
> +	time.sleep(10)
> +      	self.assertEqual("Clicked", self.marionette.execute_script("return document.getElementById('scroll').innerHTML;"))

I'd like to see some testing of the visibility exceptions. Can you add some?

::: testing/marionette/client/marionette/tests/unit/unit-tests.ini
@@ +44,5 @@
>  [test_timeouts.py]
>  b2g = false
>  
> +[test_touch.py]
> +b2g = false

This is not right. test_touch.py is meant to run on b2g only, so here you want:
b2g = true
browser = false

::: testing/marionette/client/marionette/www/testTouch.html
@@ +14,5 @@
> +  <h1 id="testh1">Test Page</h1>
> +  <script type="text/javascript">
> +    window.ready = true;
> +    setTimeout(addDelayedElement, 1000);  
> +    //window.addEventListener("mousedown", function() { alert('got touchend!');});

can you remove this line?

@@ +199,5 @@
> +4:00-5:30 M-TH: You can email Cheryl Forshee and she will try to get a hold of the night time janitorial supervisor to make sure they locate it and clean that evening (there are no night time janitors on Friday or Saturday).
> +
> +After 5:30 M-TH: You can leave a message with the Answering Service (408) 873-0121, and though it won’t be considered an emergency, once we receive the message we will attempt to get a hold of the night time janitorial supervisor (there are no night time janitors on Friday or Saturday)
> +
> +After 4:00 Friday through Sunday afternoon: Janitorial only cleans the building Sunday through Thursday nights, so it could cost extra if they have to send a janitor to the building when not scheduled. Still leave a message with the After Hours answering service and we’ll see what can be done before Sunday night when janitors return.</p>

Um, why is there all this text (lines 42-203)? It doesn't serve any purpose and if we didn't write this text ourselves, we shouldn't publish it under the MPT license. This really shouldn't be here.

::: testing/marionette/marionette-listener.js
@@ +631,5 @@
> +    }
> +  }
> +}
> +
> +function coordinates(target, x0, y0, x1, y1) {

I'd also add a docstring describing what this function does, since it's pretty hard to consume for people who haven't seen it.

@@ +727,5 @@
> +    var scroll = elementInViewport(el);
> +    if (!scroll){
> +      sendError("Element is not currently visible and may not be manipulated", 11, null, command_id);
> +      return;
> +    }

Hmm a good refactor would be to put these visible exception checks in a helper function, so you don't need to duplicate code between singleTap and doubleTap. You'll also need this code for press and hold.

@@ +736,5 @@
> +    sendOk(msg.json.command_id);
> +  }
> +  catch (e) {
> +    sendError(e.message, e.code, e.stack, msg.json.command_id);
> +    return;

You don't need the return here

@@ +758,5 @@
> +    if (el.scrollIntoView){
> +      el.scrollIntoView(true);
> +    }
> +    var scroll = elementInViewport(el);
> +    if (!sroll){

typo here

@@ +776,5 @@
> +    sendOk(msg.json.command_id);
> +  }
> +  catch (e) {
> +    sendError(e.message, e.code, e.stack, msg.json.command_id);
> +    return;

no need for return here
Comment 13 Ann(Yiming) 2013-01-30 17:16:54 PST
Created attachment 708401 [details] [diff] [review]
smallFixes
Comment 14 Ann(Yiming) 2013-01-31 08:18:45 PST
Created attachment 708600 [details] [diff] [review]
One more formatting fix
Comment 15 Malini Das [:mdas] - Away, not checking bugmail 2013-01-31 09:07:40 PST
Comment on attachment 708600 [details] [diff] [review]
One more formatting fix

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

Awesome, just one more nit to fix :)

::: testing/marionette/client/marionette/www/testTouch.html
@@ +39,5 @@
> +  <input name="myInput" type="text" value="asdf"/>
> +  <input name="myCheckBox" type="checkbox" />
> +  <h2 id="testh2" style="visibility: hidden" class="linkClass">Hidden</h2>
> +  <h3 id="testh3">Voluntary Termination</h3>
> +  <br style="margin-bottom:600px;"/>

nice!

::: testing/marionette/marionette-listener.js
@@ +750,5 @@
> +    el = elementManager.getKnownElement(msg.json.value, curWindow);
> +    let visResult = checkVisible(el, command_id);
> +    if (!visResult) {
> +      return;
> +    }

ah, a more concise way of doing this is to have checkVisible return the boolean, and then do sendError here on line 753 if we get false.

That way, there's less code duplication in checkVisible and checkVisible doesn't throw errors on our behalf (it may be the case in the future that we just want to check the visibility of an element and not throw an error, for example).

Also, for line 753 and 775, you can do:

if (!checkVisible(el, command_id)) {
  //sendError code.
}

since we don't want to really store that boolean in a variable since we only need to check it once.
Comment 16 Ann(Yiming) 2013-01-31 09:44:28 PST
Created attachment 708651 [details] [diff] [review]
checkVisible
Comment 17 Malini Das [:mdas] - Away, not checking bugmail 2013-01-31 10:33:34 PST
Comment on attachment 708651 [details] [diff] [review]
checkVisible

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

yay, I'll land this for you
Comment 18 Malini Das [:mdas] - Away, not checking bugmail 2013-01-31 10:36:51 PST
landed on inbound:

https://hg.mozilla.org/integration/mozilla-inbound/rev/ccdcf297a43f
Comment 19 Ryan VanderMeulen [:RyanVM] 2013-02-01 13:23:37 PST
https://hg.mozilla.org/mozilla-central/rev/ccdcf297a43f
Comment 20 Malini Das [:mdas] - Away, not checking bugmail 2013-02-12 10:00:54 PST
This will be very helpful for testing against mozilla-b2g18. It only affects the Marionette test framework.
Comment 21 Malini Das [:mdas] - Away, not checking bugmail 2013-02-13 13:18:23 PST
Comment on attachment 708651 [details] [diff] [review]
checkVisible

Review of attachment 708651 [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 22 Lukas Blakk [:lsblakk] use ?needinfo 2013-02-20 08:26:55 PST
Comment on attachment 708651 [details] [diff] [review]
checkVisible

Approving for uplift to v1-train since this is marionette only.
Comment 23 Malini Das [:mdas] - Away, not checking bugmail 2013-02-20 12:14:33 PST
(In reply to Lukas Blakk [:lsblakk] from comment #22)
> Comment on attachment 708651 [details] [diff] [review]
> checkVisible
> 
> Approving for uplift to v1-train since this is marionette only.

Thanks lsblakk!

Landed on mozilla-b2g18:
https://hg.mozilla.org/releases/mozilla-b2g18/rev/b31a459d68a4
Comment 24 Jonathan Griffin (:jgriffin) 2013-05-01 14:41:29 PDT
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/d22474115087

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