847758 - Need to be able to fire touchcancel for single action chains and element touch commands

Status: RESOLVED FIXED

(Testing :: Marionette, defect)

Description

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

We'll need to be able to fire the 'touchcancel' event for our touch automation.

For action chains:

action = Action()
action.press(element).cancel()
action.perform()

and:

element.press()
element.cancel_touch()

should both fire 'touchstart' on element, and then 'touchcancel'.

perhaps we should use a better name than cancel_touch() for the webelement command. Open to suggestions!

Comment 1

[Ann(Yiming)]

cancel_touch needs to consume a touchId, similar with release.
So we need 
touch_id = element.press()
element.cancel_touch(touch_id)

Comment 2

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

Right, good catch!

Comment 3

[Ann(Yiming)]

Attachment: touchCancel

Bug 841101 (multi-touch) needs to land first.

Comment 4

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

Comment on attachment 721802 [details] [diff] [review]
touchCancel

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

Cool, I just have some small improvement requests.

::: testing/marionette/client/marionette/www/testAction.html
@@ +18,5 @@
>    <!-- "mozLinkStart" and "mozLinkEnd" work together to perform touchdown on mozLinkStart, horizontal move and then touchup on mozLinkEnd -->  
>    <button id="mozLinkStart" style="position:absolute;left:10px;top:200px;" type="button" allowevents=true>Press</button>
>    <button id="mozLinkEnd" style="position:absolute;left:80px;top:200px;" type="button" allowevents=true>Release</button>
> +  <!-- "mozLinkHere" listens for a touchdown and touchcancel -->
> +  <button id="mozLinkHere" style="position:absolute;left:0px;top:255px;" type="button" allowevents=true>Button4</button>

Mind changing the name to 'mozLinkCancel'?  Makes it easier to remember what it listens for.

@@ +100,5 @@
> +
> +    function changeStartText() {
> +      var fourth = document.getElementById("mozLinkHere");
> +      fourth.innerHTML = "Start";
> +    }

Instead of creating changeStartText() specific to "mozLinkHere", you can update changePressText to be a helper method by making it accept a string. You can use that string to get the element you want and change its innerHTML.

@@ +110,5 @@
> +      }
> +      else {
> +        fourth.innerHTML = "Error";
> +      }
> +    }

Same here, changeClickText() can be repurposed to accept any element id, thus removing the need for this function.

::: testing/marionette/marionette-listener.js
@@ +915,5 @@
> +      delete touchIds[id];
> +      sendOk(msg.json.command_id);
> +    }
> +    else {
> +      sendError("Element has not be pressed: InvalidElementCoordinates", 29, null, command_id);

I think we should use code 7: NoSuchElement

http://code.google.com/p/selenium/wiki/JsonWireProtocol#Response_Status_Codes

I think this is used in other functions too, I missed it during the other patch reviews. Can you change them too, in this patch?

@@ +959,5 @@
>        actions(finger,touchId, command_id, i);
>        break;
>      case 'release':
>        touch = lastTouch;
> +      lastTouch = null;

Good catch here.

Comment 5

[Ann(Yiming)]

Attachment: improvement

Comment 6

[Ann(Yiming)]

Do we need touch cancel for multi-touch chain? This bug doesn't specify that. We can create a new bug if we do.

Comment 7

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

Hmm regarding multitouch, I think it's theoretically possible, but I'm not actually sure about it. dburns?

Comment 8

[David Burns :automatedtester]

I think that we can add this even though I think if we can avoid it we should (but thats a different discussion)

Since a user could create "smear of the screen" (using an example from MDas on IRC, but other use cases could happen) that would be create a touchcancel and we should then handle appropriately.

Comment 9

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

We'll wait on multitouch implementation for either 1) feature request or 2) what the webdriver community concludes.

Comment 10

[Ann(Yiming)]

Attachment: styleFix

Comment 11

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

Comment on attachment 726776 [details] [diff] [review]
styleFix

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

comments inline

::: testing/marionette/client/marionette/www/testAction.html
@@ +45,5 @@
>      fourth.addEventListener("touchstart", changeTimePress, false);
>      fourth.addEventListener("touchend", changeTimeRelease, false);
> +    // here is fifth for a cancel
> +    fifth.addEventListener("touchstart", function(){changePressText("mozLinkCancel")}, false);
> +    fifth.addEventListener("touchcancel", function(){changeClickText("mozLinkCancel")}, false);

you've added the listener twice

::: testing/marionette/marionette-actors.js
@@ +1354,5 @@
> +   */
> +  cancelTouch: function MDA_cancelTouch(aRequest) {
> +    this.command_id = this.getCommandId();
> +    let element = aRequest.element;
> +    let touchId = aRequest.touchId;

We have to check if we're in chrome or content context before sending a response

::: testing/marionette/marionette-listener.js
@@ +893,5 @@
>        delete touchIds[id];
>        sendOk(msg.json.command_id);
>      }
>      else {
> +      sendError("Element has not been pressed: no such element", 7, null, command_id);

Ah, thanks for fixing this

@@ +915,5 @@
> +      delete touchIds[id];
> +      sendOk(msg.json.command_id);
> +    }
> +    else {
> +      sendError("An element could not be located on the page using the given search parameters", 7, null, command_id);

Can the error message be something like "Element not previously interacted with"?

@@ +959,5 @@
>        actions(finger,touchId, command_id, i);
>        break;
>      case 'release':
>        touch = lastTouch;
> +      lastTouch = null;

And thanks for fixing this too!

Comment 12

[Ann(Yiming)]

Attachment: touchCancel

Comment 13

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

landing on m-i:
https://hg.mozilla.org/integration/mozilla-inbound/rev/62bd02cebab4

Comment 14

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/62bd02cebab4

Comment 15

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

Comment on attachment 726903 [details] [diff] [review]
touchCancel

Review of attachment 726903 [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.

Comment 16

[bhavana bajaj [:bajaj]]

Comment on attachment 726903 [details] [diff] [review]
touchCancel

low risk, no user impact helps with automation for tests - approving for uplift

Comment 17

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

https://hg.mozilla.org/releases/mozilla-b2g18/pushloghtml?changeset=71f7f42dee13

Comment 18

[Jonathan Griffin (:jgriffin)]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/089e8bb9b65a