851523 - Allow users to execute action chain in segments

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect)

Description

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

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()

Comment 1

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

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.

Comment 2

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

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!

Comment 3

[David Burns :automatedtester]

(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()

Comment 4

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

(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.

Comment 5

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

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.

Comment 6

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

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

Comment 7

[Ann(Yiming)]

Attachment: actionSegments

Comment 8

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

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

Comment 9

[Ann(Yiming)]

Attachment: fix

Comment 10

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

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.

Comment 11

[Ann(Yiming)]

Attachment: modifyTest

Comment 12

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

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)?

Comment 13

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

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

Comment 14

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

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

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/fcc773013f56

Comment 16

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

https://hg.mozilla.org/releases/mozilla-b2g18/rev/4627ae79f40c

Comment 17

[Jonathan Griffin (:jgriffin)]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/6e8ba2ff90fe