845925 - Add scroll/pinch actions using touch action chains to the python touch layer.

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect)

Description

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

We'll need to implement scroll and pinch actions to python touch layer once Bug 841101 lands.

Comment 1

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

oh, and zoom.

Comment 2

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

This entails adding the 'scroll' and 'pinch' functions to MarionetteTouchMixin (in marionette_touch.py), and using MultiAction chains to perform the tasks instead. Marionette test writers can then add this mixin to their Marionette instance to get these gestures.

We can also lose the whole Mixin thing altogether and just implement these calls in the Marionette object itself. If anyone wants to implement their own gestures with the same names, they can create their own mixin to override the methods. This seems much easier, since these are helpful methods and I doubt they'll be overridden. 

Thoughts, jgriffin, dburns?

Comment 3

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

(In reply to Malini Das [:mdas] from comment #2)
> This entails adding the 'scroll' and 'pinch' functions to
> MarionetteTouchMixin (in marionette_touch.py), and using MultiAction chains
> to perform the tasks instead. Marionette test writers can then add this
> mixin to their Marionette instance to get these gestures.
> 
> We can also lose the whole Mixin thing altogether and just implement these
> calls in the Marionette object itself. If anyone wants to implement their
> own gestures with the same names, they can create their own mixin to
> override the methods. This seems much easier, since these are helpful
> methods and I doubt they'll be overridden. 
> 
> Thoughts, jgriffin, dburns?

Just to clarify, it will be added to the Marionette client code (marionette.py). There will be no changes to the server (it will just deal with pure action chain commands)

Comment 4

[Jonathan Griffin (:jgriffin)]

Yes, I think adding it to marionette.py makes sense.

Comment 5

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

great, then we for pinch/zoom can do something like:

pinch(self, element, pixel_distance, duration=200)

where pixel distance can be the distance from the top and bottom-right corners of the element to each starting finger position. 

or:

pinch(self, element, x1, y1, x2, y2, x3, y3, x4, y4, duration=200)

where (x1, y2) and (x2, y2) are the starting positions of the fingers, and (x3, y3), (x4,y4) are the end positions.

For "scroll" we can reuse flick's signature:

flick(self, element, x1, y1, x2, y2, duration=200)

but it might be nice to have:

flick(self, element, distance, direction, duration=200)

feedback?

Comment 6

[David Burns :automatedtester]

I am happy for these to go straight onto the Actions object since they are actions.

Comment 7

[Ann(Yiming)]

Attachment: scrollPatch

This is a patch only for scroll/flick. I added marionette_session as an additional parameter for flick function signature. Also, I'm thinking to move flick function from marionette object into HTMLElement since the whole action chain depends on the same target element.

Note: from my understanding, [x1, y1] and [x2, y2] are starting and ending coordinates relative to the target element, is it correct?

Comment 8

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

(In reply to Ann(Yiming) from comment #7)
> Created attachment 733139 [details] [diff] [review]
> scrollPatch
> 
> This is a patch only for scroll/flick. I added marionette_session as an
> additional parameter for flick function signature. Also, I'm thinking to
> move flick function from marionette object into HTMLElement since the whole
> action chain depends on the same target element.
> 
> Note: from my understanding, [x1, y1] and [x2, y2] are starting and ending
> coordinates relative to the target element, is it correct?
FTR, I answered offline, saying we should move this to the Action chain, and the coordinates are relative to the target element.

Comment 9

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

Comment on attachment 733139 [details] [diff] [review]
scrollPatch

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

::: testing/marionette/client/marionette/marionette.py
@@ +483,5 @@
> +        action = Actions(marionette_session)
> +        action.press(element, x1, y1)
> +        while (time < duration):
> +            time += time_increment
> +            action.move_by_offset(corx(time), cory(time))

we need to add wait()s in order for the duration time to be respected, so if we're waiting every 10ms, then we need to add waits in between each move_by_offset.

Also, it is possible that the duration will be less than the time_increment, in which case, we should just move to the location (since it's too fast)

::: testing/marionette/client/marionette/www/testAction.html
@@ +165,5 @@
>        }
>        else {
>          second.innerHTML = "Error";
>        }
> +      var scroll = document.getElementById("mozLinkScroll");

you shouldn't hijack the changeClickText function with something that isn't relevant for all cases. This just adds extra, unnecesary processing power, and is unclear. Instead, add a listener for the button you're scrolling on, and when that button receives 'touchend' then check the viewport (as its own function)

Comment 10

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

Spoke with dburns, and we agree to put smooth_scroll (https://github.com/mozilla/b2gperf/blob/master/b2gperf/gestures.py) in a helper file instead of attached to either the Marionette object or Actions. It can be in a similar gestures.py file testing/marionette/client/marionette/gestures.py

Comment 11

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

With regards to our python touch layer, it is safe to remove flick/pinch, since neither of these are dependent on the mouse event shim. Flick/pinch will send only touch events, and the mouse event shim will catch these touch events and convert them into the mouse events the app expects.

The only problem that this will cause is that we will need to update the gaia-ui-tests that are using self.marionette.flick() to use action chains:

action = Action(self.marionette)
action.flick(<params>).perform()

It looks less pretty this way, but they can maintain helper functions, or we can add a flick() command that can be imported for them in the gestures.py file, but that seems to be overkill.

Comment 12

[Jonathan Griffin (:jgriffin)]

Yes, we could always add a Marionette mixin in gaia-ui-tests that would provide this shortcut, so it could be transparent to the tests.

Comment 13

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

(In reply to Jonathan Griffin (:jgriffin) from comment #12)
> Yes, we could always add a Marionette mixin in gaia-ui-tests that would
> provide this shortcut, so it could be transparent to the tests.

Yeah, to make things easier on the tests, I'm fine with adding a helper function in the current marionette mixin for our gaia-ui-tests to use, since the MarionetteTouchMixin is used for Gaia tests specifically. This way, it will support the way the tests are currently written.

Yiming, instead of removing flick/pinch in this patch, can you update the current flick/pinch function to use action chains like in Comment 11?

Comment 14

[Ann(Yiming)]

Attachment: scroll/pinch

A couple of notes here:
1. Something is wrong with touch cancel. After touchcancel event gets fired, the page seems to be locked up and cannot be scrolled. However, the page can still receive touch events. Problem still exists by using initTouchEvent to fire touch events. Therefore, I forced Marionette to run touchcancel related tests after scrolling related tests.

2. Emulator does not support zoom in/out. I added tests in this patch and added one more flag "unagi = false" in init-tests.py and set "unagi=true" for the pinch test. The test did run within the test suite but the page didn't zoom in. Note that I have not put any assert statements for pinch test (test_marionette_multifinger.py). Will add that later when emulator supports pinch.

3. Modified Marionette Mixin flick/pinch function in this patch.

Comment 15

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

Comment on attachment 740452 [details] [diff] [review]
scroll/pinch

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

The right thing to do here is to deactivate our touchcancel tests and file a bug against it, since it is not working as expected. I've given instructions in the review.

::: testing/marionette/client/marionette/gestures.py
@@ +1,1 @@
> +from marionette import Actions

We'll need to add the Mozilla License at the head of file

@@ +1,4 @@
> +from marionette import Actions
> +
> +#axis is y or x
> +#direction is 0 for positive, and -1 for negative

we should explain thoroughly what wait period does and that it is in seconds, what increments does and what scroll_back does.

::: testing/marionette/client/marionette/marionette.py
@@ +144,5 @@
> +        time_increment = 10
> +        if time_increment >= duration:
> +            time_increment = duration
> +        move_x = time_increment*1.0/duration * (x2 - x1)
> +        move_y = time_increment*1.0/duration * (y2 - y1)

Shouldn't we be doing:

(distance to cover) / (duration/time_increment) ?

so:

(x2 - x1) / (duration/time_increment)?

Since we can only do (duration / time_increment) moves, to cover (x2 - x1) distance?

@@ +149,5 @@
> +        self.action_chain.append(['press', element, x1, y1])
> +        while (time < duration):
> +            time += time_increment
> +            self.action_chain.append(['moveByOffset', move_x, move_y])
> +            self.action_chain.append(['wait', 0.01])

0.01 should be time_increment

@@ +151,5 @@
> +            time += time_increment
> +            self.action_chain.append(['moveByOffset', move_x, move_y])
> +            self.action_chain.append(['wait', 0.01])
> +        self.action_chain.append(['release'])
> +        return self

have we updated our MDN docs with these recent changes (long_press, etc.)? we should!

@@ +474,5 @@
>      def send_mouse_event(self, send):
>          response = self._send_message('sendMouseEvent', 'ok', value=send)
>          return response
>  
> +    def pinch(self, marionette_session, element, x1, y1, x2, y2, x3, y3, x4, y4, duration=200):

Ah, I spoke with AutomatedTester and we agree to put this in the gestures.py file so we can import it from there (like smooth_scroll)

@@ +496,5 @@
> +        action1.release()
> +        action2.release()
> +        multiAction.add(action1).add(action2).perform()
> +
> +@property

spacing has to be aligned.

::: testing/marionette/client/marionette/tests/unit/unit-tests.ini
@@ +77,5 @@
>  browser = false
>  
> +[test_cancel.py]
> +b2g = true
> +browser = false

Can you add

# Cannot run cancel test yet due to Bug X
skip = true

And file a bug about the broken 'touchcancel' behaviour?

Comment 16

[Ann(Yiming)]

Attachment: fix

Comment 17

[Ann(Yiming)]

> ::: testing/marionette/client/marionette/marionette.py
> @@ +144,5 @@
> > +        time_increment = 10
> > +        if time_increment >= duration:
> > +            time_increment = duration
> > +        move_x = time_increment*1.0/duration * (x2 - x1)
> > +        move_y = time_increment*1.0/duration * (y2 - y1)
> 
> Shouldn't we be doing:
> 
> (distance to cover) / (duration/time_increment) ?
> 
> so:
> 
> (x2 - x1) / (duration/time_increment)?
> 
> Since we can only do (duration / time_increment) moves, to cover (x2 - x1)
> distance?

If you rearrange (x2 - x1) / (duration/time_increment), it's what I did in the patch :)

Comment 18

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

Comment on attachment 741424 [details] [diff] [review]
fix

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

After you change the unit-test.ini, can you confirm that it all runs as expected when you upload the next patch? Thanks!

::: testing/marionette/client/marionette/tests/unit/unit-tests.ini
@@ +56,5 @@
>  browser = false
>  
> +# Cannot run cancel test yet due to Bug 865334
> +[test_gesture.py]
> +skip = true

We should set

skip = true

for [test_cancel.py] since that's the problematic one.

Comment 19

[Ann(Yiming)]

(In reply to Malini Das [:mdas] from comment #18)
> Comment on attachment 741424 [details] [diff] [review]
> fix
> 
> Review of attachment 741424 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> After you change the unit-test.ini, can you confirm that it all runs as
> expected when you upload the next patch? Thanks!
> 
> ::: testing/marionette/client/marionette/tests/unit/unit-tests.ini
> @@ +56,5 @@
> >  browser = false
> >  
> > +# Cannot run cancel test yet due to Bug 865334
> > +[test_gesture.py]
> > +skip = true
> 
> We should set
> 
> skip = true
> 
> for [test_cancel.py] since that's the problematic one.

The problem lies in places where touchcancel gets called, and there are two test files having touchcancel. One is test_single_finger.py since that file is all about single finger action chain, the other one is test_cancel.py which tests touchcancel outside of action chain. We will get rid of test_cancel.py really soon (happens in bug 858834). 

If I set skip=true for test_single_finger.py, then all tests related to action chain cannot be tested? So I put it under test_gesture.py so we don't do another scrolling after touchcancel. 

I already tested all the changes in this patch against emulator. Should be working properly

Comment 20

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

(In reply to Ann(Yiming) from comment #17)
> > ::: testing/marionette/client/marionette/marionette.py
> > @@ +144,5 @@
> > > +        time_increment = 10
> > > +        if time_increment >= duration:
> > > +            time_increment = duration
> > > +        move_x = time_increment*1.0/duration * (x2 - x1)
> > > +        move_y = time_increment*1.0/duration * (y2 - y1)
> > 
> > Shouldn't we be doing:
> > 
> > (distance to cover) / (duration/time_increment) ?
> > 
> > so:
> > 
> > (x2 - x1) / (duration/time_increment)?
> > 
> > Since we can only do (duration / time_increment) moves, to cover (x2 - x1)
> > distance?
> 
> If you rearrange (x2 - x1) / (duration/time_increment), it's what I did in
> the patch :)

Lol, whoops, I had a feeling was doing something wrong

Comment 21

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

(In reply to Ann(Yiming) from comment #19)
> (In reply to Malini Das [:mdas] from comment #18)
> > Comment on attachment 741424 [details] [diff] [review]
> > fix
> > 
> > Review of attachment 741424 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > After you change the unit-test.ini, can you confirm that it all runs as
> > expected when you upload the next patch? Thanks!
> > 
> > ::: testing/marionette/client/marionette/tests/unit/unit-tests.ini
> > @@ +56,5 @@
> > >  browser = false
> > >  
> > > +# Cannot run cancel test yet due to Bug 865334
> > > +[test_gesture.py]
> > > +skip = true
> > 
> > We should set
> > 
> > skip = true
> > 
> > for [test_cancel.py] since that's the problematic one.
> 
> The problem lies in places where touchcancel gets called, and there are two
> test files having touchcancel. One is test_single_finger.py since that file
> is all about single finger action chain, the other one is test_cancel.py
> which tests touchcancel outside of action chain. We will get rid of
> test_cancel.py really soon (happens in bug 858834).

Ah okay, thanks

> 
> If I set skip=true for test_single_finger.py, then all tests related to
> action chain cannot be tested? So I put it under test_gesture.py so we don't
> do another scrolling after touchcancel. 

Yeah, I'm leaning more toward skipping just the cancel test here. I'll add information on how to do that for Bug 858834. Since this is the case, let's get rid of the skip flag here.

> 
> I already tested all the changes in this patch against emulator. Should be
> working properly

Great, thanks!

Comment 22

[Ann(Yiming)]

Attachment: skipCancel

Comment 23

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

Comment on attachment 742521 [details] [diff] [review]
skipCancel

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

The rest seems okay, just this one nit.

::: testing/marionette/client/marionette/tests/unit/test_single_finger.py
@@ +131,5 @@
>          action.long_press(button, 5).perform()
>          time.sleep(10)
>          self.assertEqual("ContextEnd", self.marionette.execute_script("return document.getElementById('mozLinkCopy').innerHTML;"))
>  
> +    @skip("Skipping due to Bug 865334")

shouldn't this stuff go in the patch for https://bugzilla.mozilla.org/show_bug.cgi?id=858834? I don't even see cancel being used here

Comment 24

[Ann(Yiming)]

(In reply to Malini Das [:mdas] from comment #23)
> Comment on attachment 742521 [details] [diff] [review]
> skipCancel
> 
> Review of attachment 742521 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The rest seems okay, just this one nit.
> 
> ::: testing/marionette/client/marionette/tests/unit/test_single_finger.py
> @@ +131,5 @@
> >          action.long_press(button, 5).perform()
> >          time.sleep(10)
> >          self.assertEqual("ContextEnd", self.marionette.execute_script("return document.getElementById('mozLinkCopy').innerHTML;"))
> >  
> > +    @skip("Skipping due to Bug 865334")
> 
> shouldn't this stuff go in the patch for
> https://bugzilla.mozilla.org/show_bug.cgi?id=858834? I don't even see cancel
> being used here

touchcancel gets fired implicitly in listener. If long press follows an active touch, like action.press().longPress(), we need to cancel the first press.

Comment 25

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

(In reply to Ann(Yiming) from comment #24)
> (In reply to Malini Das [:mdas] from comment #23)
> > Comment on attachment 742521 [details] [diff] [review]
> > skipCancel
> > 
> > Review of attachment 742521 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > The rest seems okay, just this one nit.
> > 
> > ::: testing/marionette/client/marionette/tests/unit/test_single_finger.py
> > @@ +131,5 @@
> > >          action.long_press(button, 5).perform()
> > >          time.sleep(10)
> > >          self.assertEqual("ContextEnd", self.marionette.execute_script("return document.getElementById('mozLinkCopy').innerHTML;"))
> > >  
> > > +    @skip("Skipping due to Bug 865334")
> > 
> > shouldn't this stuff go in the patch for
> > https://bugzilla.mozilla.org/show_bug.cgi?id=858834? I don't even see cancel
> > being used here
> 
> touchcancel gets fired implicitly in listener. If long press follows an
> active touch, like action.press().longPress(), we need to cancel the first
> press.

aha! right

Comment 26

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

landed on inbound:

https://hg.mozilla.org/integration/mozilla-inbound/rev/1a16a93a396b

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1a16a93a396b

Comment 28

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

this needs to be checked into mozilla-b2g18 with a=test-only

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/6ac1987eb6ca

Comment 30

[Ryan VanderMeulen [:RyanVM]]

Backed out for Marionette failures.
https://hg.mozilla.org/releases/mozilla-b2g18/rev/17daac8919eb

https://tbpl.mozilla.org/php/getParsedLog.php?id=22470690&tree=Mozilla-B2g18

Comment 31

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

Fixed patch (had a python2.7ism in it):

https://hg.mozilla.org/releases/mozilla-b2g18/rev/1087e3e68ef5

Comment 32

[Jonathan Griffin (:jgriffin)]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/3bbaf25ca038