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

RESOLVED FIXED in Firefox 23

Status

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: mdas, Assigned: annyang121)

Tracking

Trunk
mozilla23
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

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

Details

Attachments

(1 attachment, 3 obsolete attachments)

We'll need to implement scroll and pinch actions to python touch layer once Bug 841101 lands.
oh, and zoom.
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?
Flags: needinfo?(dburns)
(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)
Yes, I think adding it to marionette.py makes sense.
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?
Assignee: nobody → yiyang
I am happy for these to go straight onto the Actions object since they are actions.
Flags: needinfo?(dburns)
Assignee

Comment 7

6 years ago
Posted patch scrollPatch (obsolete) — Splinter Review
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?
Attachment #733139 - Flags: feedback?(mdas)
(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 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)
Attachment #733139 - Flags: feedback?(mdas) → feedback-
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
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.
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.
(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?
Flags: needinfo?
Flags: needinfo?
Assignee

Comment 14

6 years ago
Posted patch scroll/pinch (obsolete) — Splinter Review
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.
Attachment #733139 - Attachment is obsolete: true
Attachment #740452 - Flags: review?(mdas)
Assignee

Updated

6 years ago
Blocks: 858834
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?
Attachment #740452 - Flags: review?(mdas) → review-
Assignee

Comment 16

6 years ago
Posted patch fix (obsolete) — Splinter Review
Attachment #740452 - Attachment is obsolete: true
Attachment #741424 - Flags: review?(mdas)
Assignee

Comment 17

6 years ago
> ::: 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 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.
Attachment #741424 - Flags: review?(mdas) → review-
Assignee

Comment 19

6 years ago
(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
(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
(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!
Assignee

Comment 22

6 years ago
Posted patch skipCancelSplinter Review
Attachment #741424 - Attachment is obsolete: true
Attachment #742521 - Flags: review?(mdas)
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
Assignee

Comment 24

6 years ago
(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.
(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
Attachment #742521 - Flags: review?(mdas) → review+
https://hg.mozilla.org/mozilla-central/rev/1a16a93a396b
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
this needs to be checked into mozilla-b2g18 with a=test-only
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.