Closed Bug 847758 Opened 7 years ago Closed 7 years ago

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

Categories

(Testing :: Marionette, defect)

defect
Not set

Tracking

(firefox22 fixed, b2g18 fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 fixed)

RESOLVED FIXED
mozilla22
Tracking Status
firefox22 --- fixed
b2g18 --- fixed
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- fixed

People

(Reporter: mdas, Assigned: annyang121)

Details

Attachments

(1 file, 3 obsolete files)

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!
Assignee: nobody → yiyang
cancel_touch needs to consume a touchId, similar with release.
So we need 
touch_id = element.press()
element.cancel_touch(touch_id)
Right, good catch!
Attached patch touchCancel (obsolete) — Splinter Review
Bug 841101 (multi-touch) needs to land first.
Attachment #721802 - Flags: review?(mdas)
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.
Attachment #721802 - Flags: review?(mdas) → review-
Attached patch improvement (obsolete) — Splinter Review
Attachment #721802 - Attachment is obsolete: true
Attachment #725027 - Flags: review?(mdas)
Do we need touch cancel for multi-touch chain? This bug doesn't specify that. We can create a new bug if we do.
Hmm regarding multitouch, I think it's theoretically possible, but I'm not actually sure about it. dburns?
Flags: needinfo?(dburns)
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.
Flags: needinfo?(dburns)
Summary: Need to be able to fire touchcancel for action chains and element touch commands → Need to be able to fire touchcancel for single action chains and element touch commands
We'll wait on multitouch implementation for either 1) feature request or 2) what the webdriver community concludes.
Attached patch styleFix (obsolete) — Splinter Review
Attachment #725027 - Attachment is obsolete: true
Attachment #725027 - Flags: review?(mdas)
Attachment #726776 - Flags: review?(mdas)
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!
Attachment #726776 - Flags: review?(mdas) → review-
Attached patch touchCancelSplinter Review
Attachment #726776 - Attachment is obsolete: true
Attachment #726903 - Flags: review?(mdas)
Attachment #726903 - Flags: review?(mdas) → review+
https://hg.mozilla.org/mozilla-central/rev/62bd02cebab4
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
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.
Attachment #726903 - Flags: approval-mozilla-b2g18?
Comment on attachment 726903 [details] [diff] [review]
touchCancel

low risk, no user impact helps with automation for tests - approving for uplift
Attachment #726903 - Flags: approval-mozilla-b2g18? → approval-mozilla-b2g18+
You need to log in before you can comment on or make changes to this bug.