If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Allow users to execute action chain in segments

RESOLVED FIXED in Firefox 22

Status

Testing
Marionette
RESOLVED FIXED
5 years ago
5 years ago

People

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

Tracking

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

Firefox Tracking Flags

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

Details

Attachments

(1 attachment, 2 obsolete attachments)

We don't have a way to wait for an element to appear in our current action chain. If the element is not visible, and is only made visible after a gesture in the action chain, we cannot act on it. Take this, for example:

action.press(self.marionette.find_element("id","element1")).move(self.marionette.find_element("id", "element2")).release().perform()

Say element2 appears only after element1 is pressed.

Due to the order of execution, we will first evaluate the parameters before calling move(), so self.marionette.find_element("id", "element2") will be called before we add move() to the action chain. So we try to find the element before perform() is even called, hence this chain will fail.

We need to be able to call 'find_element' at the right time.

I have one suggestion:

Alter find_element so that we can do find_element("id", "element2", future=True), so that it won't try to find the element now, but will instead return a "future" object. We'd need to allow action chains to consume these "future" objects. So we can do:

element2 = self.find_element("id, "element2", future=True) # return a Future element

action.press(element1).move(element2).release().perform()
Blocks: 851004
Dburns and I were speaking and a nicer idea of breaking this down, so instead of one full action chain that can only be performed, we can do something like this:

element1 = self.marionette.find_element("id", element1")
action.press(element1).perform()
element2 = self.marionette.find_element("id", element2")
action.move(element2).release().perform()

And have it do what we expect. Currently, this won't work because when we call perform(), it will perform the whole chain, from the beginning. ie: when the second perform is called here, it will actually do: action.press(element1).move(element2).release().perform(), when all we want is action.move(element2).release().perform()

As such, we want to change what perform() does so that it doesn't duplicate actions. We also, more importantly, want to be able to reference the original touchstart event (all gestures are relative to the target of the first touchstart event), so we need to preserve that information across each segment of the chain.

Going to leave this comment here while I think how we'll need to implement it exactly.
To get this idea working:

- We'll need to change how perform() works so that it will remove all the actions it will have performed

- perform() or marionette backend will need to preserve the target element of first touchstart, and will need to keep track of the last touched coordinates (so we can do things like move_by_offset and it will operate on the last known coordinates).  

This applies to MultiAction chains too!
Summary: Need to allow users to wait for an element in action chain → Allow users to execute action chain in segments
(In reply to Malini Das [:mdas] from comment #1)
> 
> element1 = self.marionette.find_element("id", element1")
> action.press(element1).perform()
> element2 = self.marionette.find_element("id", element2")
> action.move(element2).release().perform()

Why cant we use 2 separate action chains for this type of scenario? Sorry if I am missing the obvious use case. Today is not proving to be one of my best.

e.g.

 element1 = self.marionette.find_element("id", element1")
 action1.press(element1).perform()
 element2 = self.marionette.find_element("id", element2")
 action2.move(element2).release().perform()
(In reply to David Burns :automatedtester from comment #3)
> Why cant we use 2 separate action chains for this type of scenario? Sorry if
> I am missing the obvious use case. Today is not proving to be one of my best.
> 
> e.g.
> 
>  element1 = self.marionette.find_element("id", element1")
>  action1.press(element1).perform()
>  element2 = self.marionette.find_element("id", element2")
>  action2.move(element2).release().perform()

The way a gesture works is that every touchmove after the initial touchstart is relative to the initial touchstart target (which is annoying). On a deeper level, this is  So action1 dispatches touchstart at element1, and then any subsequent touchmove has to be relative to element1. As such, action2 needs to know which gesture it's continuing and would need to know, so it can use that same target.

On a deeper level, in Gecko, we actually have to keep track of all Touch objects that are currently being executed, and use them as a base for subsequent commands.
If we allow this, we also need to think about allowing users to copy action chains, so they don't need to rebuild a new action chain every time they want to repeat an action. Filed Bug 852637 for this.
While we're at it, we should ensure that release() can't be called without press() first being called, and error handling for other state-dependent methods. Right now, if you do actions.release().perform(), we don't check if press was called, and we will eventually hit an uncaught error in marionette-listener.js
Assignee: nobody → yiyang
(Assignee)

Comment 7

5 years ago
Created attachment 727387 [details] [diff] [review]
actionSegments
Attachment #727387 - Flags: review?(mdas)
Comment on attachment 727387 [details] [diff] [review]
actionSegments

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

::: testing/marionette/client/marionette/marionette.py
@@ +13,5 @@
>  from keys import Keys
>  from errors import *
>  from emulator import Emulator
>  from geckoinstance import GeckoInstance
> +nextTouchId = 1000

server-side state should never be handled on the client side. This can easily result in duplicate touchIds. For example, what happens if first you did element.press(), and then do action.press(element2), action2.press(element3)? Then, the element.press() and press(element3) separate actions would both be assigned touchId 1001, and that's not what we want. The server should manage all state so we can enforce uniqueness.

You should get this value from the server, instead of managing it in the client. Have perform() return the value of the current touchId instead of managing it client-side.

::: testing/marionette/client/marionette/tests/unit/test_segments.py
@@ +24,5 @@
> +        testTouch = self.marionette.absolute_url("testAction.html")
> +        self.marionette.navigate(testTouch)
> +        action = Actions(self.marionette)
> +        action.release()
> +        self.assertRaises(NoSuchElementException, action.perform)

These are great tests! They can go in test_single_finger though, instead of creating a new file
Attachment #727387 - Flags: review?(mdas) → review-
(Assignee)

Comment 9

5 years ago
Created attachment 728000 [details] [diff] [review]
fix
Attachment #727387 - Attachment is obsolete: true
Attachment #728000 - Flags: review?(mdas)
Comment on attachment 728000 [details] [diff] [review]
fix

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

test_segments.py's test cases should be moved to test_single_finger.py, and remove the test_segments.py file.
(Assignee)

Comment 11

5 years ago
Created attachment 728334 [details] [diff] [review]
modifyTest
Attachment #728000 - Attachment is obsolete: true
Attachment #728334 - Flags: review?(mdas)
Attachment #728000 - Flags: review?(mdas)
Comment on attachment 728334 [details] [diff] [review]
modifyTest

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

::: testing/marionette/marionette-listener.js
@@ +932,5 @@
>    if (typeof i === "undefined") {
>      i = 0;
>    }
>    if (i == finger.length) {
> +    sendResponse({value: touchId}, command_id);

Ah, I just noticed this error.

We don't always want to send back the last used touchId. If release/touchcancel was just called, then the touchId of the gesture is no longer relevant. In this case, we want to return null, because then, perform() in Actions will set its current_id to null. If it doesn't get set to null, it will reuse an old id for the next time it does a chain.

To make sure we catch this problem if it ever comes up again, can you add another press/release chain to the Action object in test_chain and make sure it does what we expect (after line 54 in test_single_finger.py)?
Attachment #728334 - Flags: review?(mdas) → review-
Comment on attachment 728334 [details] [diff] [review]
modifyTest

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

r+ since the touchId only has to be unique when compared to other active touch ids.

I'll land this as soon as I can
Attachment #728334 - Flags: review- → review+
landed in m-i with a minor version conflict (resolved, upped version number of marionette client to 0.5.23)

https://hg.mozilla.org/integration/mozilla-inbound/rev/fcc773013f56
https://hg.mozilla.org/mozilla-central/rev/fcc773013f56
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
https://hg.mozilla.org/releases/mozilla-b2g18/rev/4627ae79f40c
status-b2g18: --- → fixed
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/6e8ba2ff90fe
status-b2g18-v1.0.0: --- → wontfix
status-b2g18-v1.0.1: --- → fixed
status-firefox22: --- → fixed
You need to log in before you can comment on or make changes to this bug.