857582 - send mouse events if we can't send touch events

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect)

Description

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

Davehunt mentioned that the gaia UI tests should be able to run in desktop b2g, where touch events cannot be generated/handled. The gaia UI tests are making use of action chains, which right now, only send touch events for gestures other than taps. To make it easier for cross platform testing, we should enable mouse action chains. 

We can do this two ways. For both cases, have the gaia UI test framework or python touch abstraction layer check if doc.createTouch exists, and if not, set some flag like self.marionette.send_mouse_event(true). Once this is set, either 1) any action chain performed will notice this flag during execution in the marionette server and send only mouse events or 2) we can implement mouse action chains (separate from touch action chains, which we will need to implement anyway) and when action chains get 'perform' called on them, before being sent to the marionette server, we can check the mouse flag, and if set, we will create a mouse event chain and dispatch that.

While 2) seems a bit complicated, I prefer it since it's non-invasive to the current touch action chain, and it will rely on a mouse action chain to execute instead of piggybacking on a touch action chain. Thoughts?

Comment 1

[David Burns :automatedtester]

since the gecko part of marionette already has access to the document, why can't we test for document.createTouch server side and then either throw when we receive touch or delegate down to just mouse events

Comment 2

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

(In reply to David Burns :automatedtester from comment #1)
> since the gecko part of marionette already has access to the document, why
> can't we test for document.createTouch server side and then either throw
> when we receive touch or delegate down to just mouse events

Of course, that's much better.

Comment 3

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

One byproduct this approach has is that it effectively enables people to write mouse interaction chains in two ways. One way would be to use the same methods described in a touch Action since it will fall back on mouse events, and the other would be to use the mouse Actions described herehttp://code.google.com/p/selenium/source/browse/java/client/src/org/openqa/selenium/interactions/Actions.java?name=selenium-2.5.0&r=4b40f0a1888c58f746fdecb1731c033ce9bc57fc. For example, one could write action.tap() and action.click() and both would work the same way in a mouse-only environment. We're providing more than one way to do the same thing, which is an interesting thing to consider when writing the interactions spec.

Comment 4

[David Burns :automatedtester]

(In reply to Malini Das [:mdas] from comment #3)
> For example, one could write
> action.tap() and action.click() and both would work the same way in a
> mouse-only environment. We're providing more than one way to do the same
> thing, which is an interesting thing to consider when writing the
> interactions spec.

This is indeed and I dont know if you remember the conversation we had on vidyo re: spec about should we have just click() or should we have click() AND tap()...

Comment 5

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

(In reply to David Burns :automatedtester from comment #4)
> (In reply to Malini Das [:mdas] from comment #3)
> > For example, one could write
> > action.tap() and action.click() and both would work the same way in a
> > mouse-only environment. We're providing more than one way to do the same
> > thing, which is an interesting thing to consider when writing the
> > interactions spec.
> 
> This is indeed and I dont know if you remember the conversation we had on
> vidyo re: spec about should we have just click() or should we have click()
> AND tap()...

Not quite, I have conflicting notes. One says that Simon Stewart prefers click() to do the right thing per environment, then one note says "we should keep the separate since we shouldn't treat Desktop the same as mobile as we've had problems with this before, therefore we should treat them differently.". Seems like a discussion point on the draft?

Comment 6

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

So, before we start work on this, I'd like to list some key differences between this way of executing mouse event versus mouse-specific action chains. 

What we're trying to do here is translating single finger gestures over to mouse events. This means that when we do press/release, we're implicitly using the left mouse button.

This also means that a 'long click' will trigger a 'contextmenu' as expected for touch events, as opposed to traditional desktop environments where we fire this via a right click and not by holding down the left mouse button.

So something like:
actions.press(element).wait(5).release().perform() 

will fire the 'contextmenu' event for mouse-only environments.

Comment 7

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

Attachment: Refactor and add touch->mouse translation

Since this feature needed some control flow reworking, I went and refactored a bunch of the existing code. 

I've tested this patch against the emulator, desktop b2g and unagi, and it works as expected. I tested the gaia-ui-tests that use action chains against a desktop b2g build and the touch->mouse translation worked as expected as well. I'll update the marionette_touch.py layer to use sendMouseEventsOnly and our marionette commands instead of synthetic_gestures.js when addressing Bug 845849 since there are some unrelated changes I'll have to make and further testing to get that done, and I'd rather separate these concerns.

The only weird thing I noticed is, while testing against our b2gperf scrollfps tests, the actions were working well, but a marionetteScriptFinished call was timing out, but this was after all the gestures completed. Because of this, I'm asking for f?, since this needs investigation, and this patch may change if it is the root cause.

Running the patch on try:
https://tbpl.mozilla.org/?tree=Try&rev=a254a1aa3337

Comment 8

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

Attachment: Refactor and add touch->mouse translation

Found some small nits when looking at the patch in splinter review mode. Cleaned them up.

Running the new one in try in case a change broke something I didn't test:
 https://tbpl.mozilla.org/?tree=Try&rev=21b1b902e551

Comment 9

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

Comment on attachment 747438 [details] [diff] [review]
Refactor and add touch->mouse translation

bah, habits:)

Comment 10

[Jonathan Griffin (:jgriffin)]

Comment on attachment 747438 [details] [diff] [review]
Refactor and add touch->mouse translation

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

Nice!  I appreciate all the code simplification.  I didn't review all the tests in great detail, but I like their overall structure.  I'll review more closely when r? is flagged.

Currently, it looks like only the tests use send_mouse_events_only(); is the intention that gaia-ui-tests will be the only consumer of this at present?  Instead of making a separate Marionette command for this, would it make sense to make this an optional parameter to action.perform()?

::: testing/marionette/client/marionette/tests/unit/test_gesture.py
@@ +22,5 @@
> +                  left >= window.pageXOffset &&
> +                  (top + height) <= (window.pageYOffset + window.innerHeight) &&
> +                  (left + width) <= (window.pageXOffset + window.innerWidth));
> +        };
> +    """

Could we use the more compact form from http://mxr.mozilla.org/mozilla-central/source/testing/marionette/marionette-listener.js#782?

::: testing/marionette/client/marionette/tests/unit/test_marionette_touch.py
@@ +31,3 @@
>  
> +    def test_tap_mouse_shim(self):
> +        self.marionette.execute_script("window.wrappedJSObject.MouseEventShim = 'mock value';") 

extra whitespace at end of line

@@ +41,5 @@
> +
> +    """
> +    #Enable this when we have logic in double_tap to handle the shim
> +    def test_double_tap_mouse_shim(self):
> +        self.marionette.execute_script("window.wrappedJSObject.MouseEventShim = 'mock value';") 

extra whitespace at end of line

Comment 11

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

(In reply to Jonathan Griffin (:jgriffin) from comment #10)
> Comment on attachment 747438 [details] [diff] [review]
> Refactor and add touch->mouse translation
> 
> Review of attachment 747438 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Nice!  I appreciate all the code simplification.  I didn't review all the
> tests in great detail, but I like their overall structure.  I'll review more
> closely when r? is flagged.
> 
> Currently, it looks like only the tests use send_mouse_events_only(); is the
> intention that gaia-ui-tests will be the only consumer of this at present?

Yes, for desktop B2G and for dealing with the mouse event shim in MarionetteTouchMixin.
 
> Instead of making a separate Marionette command for this, would it make
> sense to make this an optional parameter to action.perform()?

Ah, it's not just for action chains., we're allowing people to translate marionette's singleTap command to mouse events too (element.tap()). 

> 
> ::: testing/marionette/client/marionette/tests/unit/test_gesture.py
> @@ +22,5 @@
> > +                  left >= window.pageXOffset &&
> > +                  (top + height) <= (window.pageYOffset + window.innerHeight) &&
> > +                  (left + width) <= (window.pageXOffset + window.innerWidth));
> > +        };
> > +    """
> 
> Could we use the more compact form from
> http://mxr.mozilla.org/mozilla-central/source/testing/marionette/marionette-
> listener.js#782?
> 
> ::: testing/marionette/client/marionette/tests/unit/test_marionette_touch.py
> @@ +31,3 @@
> >  
> > +    def test_tap_mouse_shim(self):
> > +        self.marionette.execute_script("window.wrappedJSObject.MouseEventShim = 'mock value';") 
> 
> extra whitespace at end of line
> 
> @@ +41,5 @@
> > +
> > +    """
> > +    #Enable this when we have logic in double_tap to handle the shim
> > +    def test_double_tap_mouse_shim(self):
> > +        self.marionette.execute_script("window.wrappedJSObject.MouseEventShim = 'mock value';") 
> 
> extra whitespace at end of line

Yup, thanks for all these, I've addressed them in my next patch

Comment 12

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

Attachment: Refactor and add touch->mouse translation

The b2gperf tests were already broken, so this patch doesn't affect that issue. There was a test failure in try, but I can't view it now that try's closed. I'll rerun it when it's open

Comment 13

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

I've also modified a test that was intermittently failing in this patch, so perhaps that was the failure on try.

Comment 14

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

Now that try is back, I checked the test that failed, and it was the test that I fixed with this patch. It required a little bit of a wait.

Comment 15

[Jonathan Griffin (:jgriffin)]

Comment on attachment 750173 [details] [diff] [review]
Refactor and add touch->mouse translation

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

I know there's another patch coming, but I'd almost completed this review, so I'll go ahead and publish it.

Aside from the nits below, I'd rather not rely on time.sleep() in tests unless we really have to.  It will either bite us with intermittent failures, or if the sleep is long enough, just be annoying when running the tests locally.

There are multiple ways around this, I think, but here's one idea.  In appendText() in testAction.html, how about setting the "class" attribute of some dummy div element to the id of the target plus text (e.g., "button1-click")?  That way, you could set_search_timeout to something reasonable to allow for occasionally pokey slaves and then call find_element using the expected class (e.g., "button1-click", or maybe "button1_click"...not sure if "-" is a legal character in class values) to wait until the action has completed.

Another option is to use a waitFor() call in test_execute_async_script to wait for the innerHTML of button1, etc, to reach the expected state.

::: testing/marionette/marionette-actors.js
@@ +1310,3 @@
>     this.command_id = this.getCommandId();
>     if (this.context == "chrome") {
>       this.sendError("Not in Chrome", 500, null, this.command_id);

I know this line isn't part of your patch, but I think we should change the error to something a bit more descriptive like "Command send_mouse_events_only not available in chrome context".

::: testing/marionette/marionette-listener.js
@@ +578,2 @@
>   */
> +function mousetap(doc, duration, x, y) {

We never use 'duration'...is that intentional?  Should we remove it?

@@ +585,5 @@
>  
>  /**
> + * This function generates a pair of coordinates relative to the viewport given a
> + * target element and coordinates relative to that element's top-left corner.
> + * @param 'x', and 'y' are the relative to the target. 

nit: extra space at end of line

@@ +716,5 @@
> +      isTap = false;
> +      let event = curWindow.document.createEvent('HTMLEvents');
> +      event.initEvent('contextmenu',
> +                      true,
> +                      true);

nit: probably no need to wrap these lines

@@ +726,5 @@
> +      }
> +      target.dispatchEvent(event);
> +      break;
> +    default:
> +      throw {message:"Unknown event type", code: 500, stack:null};

For clarity, how about "Unknown event type: " + type

@@ +758,2 @@
>   */
>  function createATouch(el, corx, cory, id) {

For consistency with other functions, we should probably use touchId instead of id in the parameter list.

@@ +862,5 @@
>      if (touchId == null) {
>        touchId = nextTouchId++;
>      }
> +    if (!curWindow.document.createTouch) {
> +      mouseEventsOnly = true;

This will override a value set by send_mouse_events_only; is that what we want?

Comment 16

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

(In reply to Jonathan Griffin (:jgriffin) from comment #15)
> @@ +862,5 @@
> >      if (touchId == null) {
> >        touchId = nextTouchId++;
> >      }
> > +    if (!curWindow.document.createTouch) {
> > +      mouseEventsOnly = true;
> 
> This will override a value set by send_mouse_events_only; is that what we
> want?

Yes, if document.createTouch isn't available, then it means we're in a non-touch-enabled environment (like desktop b2g) and so we want to only send mouse events.

Comment 17

[Jonathan Griffin (:jgriffin)]

Comment on attachment 750173 [details] [diff] [review]
Refactor and add touch->mouse translation

Unflagging since another patch is coming.

Comment 18

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

Attachment: Refactor and add touch->mouse translation v2.0

I've addressed the issues you brought up in this patch, but I did have to comment out tests after I noticed some bad behaviour due to move/moveByOffset.

I spent too long trying to solve the moveByOffset/screen scroll problem, so I filed Bug 874914 and commented out test_chain_flick() and test_move_by_offset to deal with the problem outside of this bug. It's not related to my changes, since it occurs without my changes.

Comment 19

[Jonathan Griffin (:jgriffin)]

Comment on attachment 752807 [details] [diff] [review]
Refactor and add touch->mouse translation v2.0

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

Awesome, thanks!

::: testing/marionette/client/marionette/marionette_test.py
@@ +159,5 @@
> +            try:
> +                value = method(self.marionette)
> +                if value:
> +                    return value
> +            except NoSuchElementException:

I'm not sure which exceptions, if any, we should automatically catch.  Should we include StaleElementException, and/or ElementNotVisibleException?  Maybe dburns can comment on what would be most WebDriver-ish.

::: testing/marionette/client/marionette/tests/unit/test_gesture.py
@@ +12,5 @@
> +          return (rect.top >= window.pageYOffset &&
> +                 rect.left >= window.pageXOffset &&
> +                 rect.bottom <= (window.pageYOffset + window.innerHeight) &&
> +                 rect.right <= (window.pageXOffset + window.innerWidth)
> +         );   

nit: extra whitespace here

Comment 20

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

Attachment: Refactor and add touch->mouse translation v3.0

(In reply to Jonathan Griffin (:jgriffin) from comment #19)
> Comment on attachment 752807 [details] [diff] [review]
> Refactor and add touch->mouse translation v2.0
> 
> Review of attachment 752807 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Awesome, thanks!
> 
> ::: testing/marionette/client/marionette/marionette_test.py
> @@ +159,5 @@
> > +            try:
> > +                value = method(self.marionette)
> > +                if value:
> > +                    return value
> > +            except NoSuchElementException:
> 
> I'm not sure which exceptions, if any, we should automatically catch. 
> Should we include StaleElementException, and/or ElementNotVisibleException? 
> Maybe dburns can comment on what would be most WebDriver-ish.

You're right, we shouldn't be catching exceptions here. If the user of this function wants to catch an exception, they can do so in their lambda function.

Also, in light of https://bugzilla.mozilla.org/show_bug.cgi?id=845849#c7, I've rewritten the patch to remove the send_mouse_events_only() function, since we won't need to explicitly set it anymore. We did that only in the case of the gaia shim, and we don't need to detect the shim anymore. As such, we can now safely determine if we should dispatch touch or mouse events from gecko side, using by just checking if createTouch is there.

I'm running this updated patch through try: https://tbpl.mozilla.org/?tree=Try&rev=0a31eb125a2a

Comment 21

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

Attachment: Refactor and add touch->mouse translation v3.0

bah, I neglected to hg add some new test files. Re running through try:
https://tbpl.mozilla.org/?tree=Try&rev=cd80a1145fee

I'm obsoleting v2.0, since bsilverberg and zac are going through and converting the calls from MarionetteTouchMixin's tap() to marionette's tap. If there are problems with this that aren't easily solvable, then they can keep using the old MarionetteTouchMixin (with synthetic_gestures.js, ie: it won't use sendMouseEventsOnly) and then prioritize Bug 869034 to address marionette-side tap() issues. If we address all of them, we can then get rid of their use of MarionetteTouchMixin.

Comment 22

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

try's not happy with desktop. Will have to rebuild my local m-c for testing and upload a new patch.

Comment 23

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

Attachment: Refactor and add touch->mouse translation v3.0

Testing out a new patch on try:
https://tbpl.mozilla.org/?tree=Try&rev=a47d346e92aa

will r? if it goes green

Comment 24

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

Attachment: Refactor and add touch->mouse translation v3.0

Fixed try failure by having less strict event checking. It should prevent any further intermittent failures.

testing new patch on try: https://tbpl.mozilla.org/?tree=Try&rev=071ee7c1e428

will r? if tests look good.

Comment 25

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

Comment on attachment 753913 [details] [diff] [review]
Refactor and add touch->mouse translation v3.0

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

try's running green

Comment 26

[Jonathan Griffin (:jgriffin)]

Comment on attachment 753913 [details] [diff] [review]
Refactor and add touch->mouse translation v3.0

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

Cool.  I think single_finger_functions.py should live next to the tests, although this means you'd have to patch sys.path in order to import it, like:

import sys
sys.path.append(os.path.dirname(__file__))
from single_finger_functions import *

Comment 27

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

Attachment: Refactor and add touch->mouse translation v3.0

Updated patch with recommended changes. carrying r+ forward.

pushed to m-i:
https://hg.mozilla.org/integration/mozilla-inbound/rev/be20fd09cec6

Comment 28

[Phil Ringnalda (:philor)]

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

Comment 29

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

We need to land this in v1.0.1 and mozilla-b2g18 to improve testing, with a=test-only

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/5930d2b97ff7
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/3ce22ad62a4b