Closed
Bug 847758
Opened 10 years ago
Closed 10 years ago
Need to be able to fire touchcancel for single action chains and element touch commands
Categories
(Testing :: Marionette, defect)
Testing
Marionette
Tracking
(firefox22 fixed, b2g18 fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 fixed)
RESOLVED
FIXED
mozilla22
People
(Reporter: mdas, Assigned: annyang121)
Details
Attachments
(1 file, 3 obsolete files)
16.12 KB,
patch
|
mdas
:
review+
bajaj
:
approval-mozilla-b2g18+
|
Details | Diff | Splinter Review |
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!
Reporter | ||
Updated•10 years ago
|
Assignee: nobody → yiyang
Assignee | ||
Comment 1•10 years ago
|
||
cancel_touch needs to consume a touchId, similar with release. So we need touch_id = element.press() element.cancel_touch(touch_id)
Reporter | ||
Comment 2•10 years ago
|
||
Right, good catch!
Assignee | ||
Comment 3•10 years ago
|
||
Bug 841101 (multi-touch) needs to land first.
Attachment #721802 -
Flags: review?(mdas)
Reporter | ||
Comment 4•10 years ago
|
||
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-
Assignee | ||
Comment 5•10 years ago
|
||
Attachment #721802 -
Attachment is obsolete: true
Attachment #725027 -
Flags: review?(mdas)
Assignee | ||
Comment 6•10 years ago
|
||
Do we need touch cancel for multi-touch chain? This bug doesn't specify that. We can create a new bug if we do.
Reporter | ||
Comment 7•10 years ago
|
||
Hmm regarding multitouch, I think it's theoretically possible, but I'm not actually sure about it. dburns?
Flags: needinfo?(dburns)
Comment 8•10 years ago
|
||
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)
Reporter | ||
Updated•10 years ago
|
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
Reporter | ||
Comment 9•10 years ago
|
||
We'll wait on multitouch implementation for either 1) feature request or 2) what the webdriver community concludes.
Assignee | ||
Comment 10•10 years ago
|
||
Attachment #725027 -
Attachment is obsolete: true
Attachment #725027 -
Flags: review?(mdas)
Attachment #726776 -
Flags: review?(mdas)
Reporter | ||
Comment 11•10 years ago
|
||
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-
Assignee | ||
Comment 12•10 years ago
|
||
Attachment #726776 -
Attachment is obsolete: true
Attachment #726903 -
Flags: review?(mdas)
Reporter | ||
Updated•10 years ago
|
Attachment #726903 -
Flags: review?(mdas) → review+
Reporter | ||
Comment 13•10 years ago
|
||
landing on m-i: https://hg.mozilla.org/integration/mozilla-inbound/rev/62bd02cebab4
Comment 14•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/62bd02cebab4
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
Reporter | ||
Comment 15•10 years ago
|
||
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 16•10 years ago
|
||
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+
Reporter | ||
Comment 17•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-b2g18/pushloghtml?changeset=71f7f42dee13
status-b2g18:
--- → fixed
You need to log in
before you can comment on or make changes to this bug.
Description
•