Closed Bug 850819 Opened 7 years ago Closed 7 years ago

Action Chains/element gestures and touch/mouse events

Categories

(Testing :: Marionette, defect)

x86
macOS
defect
Not set

Tracking

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

RESOLVED FIXED
mozilla23
Tracking Status
firefox21 --- wontfix
firefox22 --- wontfix
firefox23 --- fixed
b2g18 --- fixed
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- fixed

People

(Reporter: mdas, Assigned: annyang121)

References

Details

Attachments

(1 file, 5 obsolete files)

Currently, our action chains only dispatch touch events. This was by design as the dispatching of mouse events is optional at this point, and dependent on the platform. 

For Gaia apps that do not use the mouse-event shim, this approach does not work, since the touch event has to be accompanied by mouse events, ie: [touchstart, touchmove*, touchend, mousemove, mousedown, mouseup, click] in this order.

To do this, we can have a setting in marionette (say 'send_mouse_gesture') and if set to true, it will send the mouse gesture with the touch events for any interaction (element press/release, or action/multi action chains).  Alternatively, we can accept a parameter to each method, and if it is set to true, it will dispatch the mouse event, but this is more cumbersome and I do not see it as that useful. An abstraction layer like marionette_touch.py that we have now can be used to detect if the shim is in place, and send the correct events accordingly.

Any other suggestions/thoughts?
Flags: needinfo?(dburns)
(In reply to Malini Das [:mdas] from comment #0)
do this, we can have a setting in marionette (say 'send_mouse_gesture')
> and if set to true, it will send the mouse gesture with the touch events for
> any interaction (element press/release, or action/multi action chains).

This should be set to True by default since the default behavior in Gecko is to create the touch/mouse/click chain of events. We will want to mimic that chain.
This is also true of the element
Summary: Action Chains and touch/mouse events → Action Chains/element gestures and touch/mouse events
(In reply to Malini Das [:mdas] from comment #0)
> To do this, we can have a setting in marionette (say 'send_mouse_gesture')
> and if set to true, it will send the mouse gesture with the touch events for
> any interaction (element press/release, or action/multi action chains). 

dburns begrudgingly accepted this via email discussion. We'll have to start work on adding a setting in marionette, lets call it "send_mouse_gesture", and have the element gestures/action chains check that value, and send the right chain of events.
Flags: needinfo?(dburns)
To do this, first we need to modify marionette to take a command called send_mouse_event, so we can do

self.marionette.send_mouse_event(False)

and it will work like set_search_timeout, where it will set a value in marionette-listener. We want teh default value to be True.

After this, we'll need to have all the element gesture methods (single_tap/double_tap/press/release) send the related mouse events if this setting is true. Nothing needs to be changed in marionette.py, just in marionette-listener, so for each of these functions, we check if we shoudl send the related mouse event. 

Same idea goes for action chains.
Assignee: nobody → yiyang
So after reading this insanely helpful file http://mxr.mozilla.org/mozilla-central/source/dom/ipc/TabChild.cpp#1636 and pinging mbrubeck & cpeterson, we now have answers about when to dispatch mouse events:

When do we send mouse events?
- *only* for tap. That is, a quick tap. If you're doing a long press, you actually don't send mouse events. Only 'contextmenu' is fired. No subsequent mouse events will be fired.

How does this play into gaia?
- Well, we need to consider the mouse_event_shim. If the shim is there, until we dispatch trusted mouse clicks, we can *only* send touch events. In our python abstraction layer used for gaia testing (mozilla-central/testing/marionette/client/marionette/marionette_touch.py), we detect if the mouse_event_shim is being used, and then tell synthetic_gestures to do the right tap event (either the chain or just touch events). So if the shim is there, we can dispatch a trusted mouse event, if it is not, then we can use our tap() function.

Interesting cases:
action.press(element).release() 

here, we'd like it to do the right thing, which is think of it as a tap (ie: dispatch mouse events), since this is essentially a tap. 

If we have a wait in between press and release, this may cause a contextmenu to fire. If it does, we can cancel the mouse event dispatching. If no contextmenu event is fired, then it hasn't been held down long enough, which makes it a tap gesture, which means we will have to dispatch the mouse event.
Attached patch mouseEvent (obsolete) — Splinter Review
Attachment #732688 - Flags: review?(mdas)
No longer blocks: 850651
note: waiting for next patch, since a mouse double click has to be in the form of two touch/mouse/click chains, one after another.
Attached patch updateDoubleTap (obsolete) — Splinter Review
Attachment #732688 - Attachment is obsolete: true
Attachment #732688 - Flags: review?(mdas)
Attachment #733593 - Flags: review?(mdas)
Comment on attachment 733593 [details] [diff] [review]
updateDoubleTap

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

great first patch, but there are some small nites, but the biggest change will be using the utils variable.

::: testing/marionette/client/marionette/tests/unit/test_single_finger.py
@@ +75,5 @@
> +        self.marionette.navigate(testTouch)
> +        self.marionette.send_mouse_event(True)
> +        action = Actions(self.marionette)
> +        button = self.marionette.find_element("id", "mozMouse")
> +        action.press(button).wait(0.74).release().perform()

since we don't want to trigger contextmenu firing, let this be a lot shorter, like 0.1, in case it gets slowed down when running in CI

::: testing/marionette/client/marionette/www/testAction.html
@@ +60,5 @@
> +    sixth.addEventListener("mouseup", function(){changeMouseText("MouseUp")}, false);
> +    sixth.addEventListener("click", function(){changeMouseText("MouseClick")}, false);
> +    function changeMouseText(strId) {
> +      var mouse = document.getElementById("mozMouse");
> +      switch(strId) {

There should be a default case which sets the innerHTML to something not in the cases, like 'Error', so that when something does get fired in the wrong order, the html makes a note of it, and if it runs though the switch block, it won't be caught by any of the cases by accident.

::: testing/marionette/marionette-listener.js
@@ +571,5 @@
> + */
> +function sendMouseEvent(msg) {
> +  let command_id = msg.json.command_id;
> +  try {
> +    mouseEvent = msg.json.value;

We should check if this is a valid boolean value, then assign mouseEvent. If it isn't, we should throw an error.

@@ +573,5 @@
> +  let command_id = msg.json.command_id;
> +  try {
> +    mouseEvent = msg.json.value;
> +    sendOk(msg.json.command_id);
> +  }

Wrapping this in a try/catch statement doesn't buy us anything, since setting mouseEvent/sendOk should not generate exceptions.

@@ +576,5 @@
> +    sendOk(msg.json.command_id);
> +  }
> +  catch (e) {
> +    sendError(e.message, e.code, e.stack, msg.json.command_id);
> +    return;

no need for a return statement here

@@ +592,5 @@
>  
>  /**
> + * This function emit mouse event
> + */
> +function emitMouseEvent(doc, type, detail, button, elClientX, elClientY) {

Can you explain what the parameters are in the docstring?

@@ +609,5 @@
> +                            false, false, false, false, // keyboard modifiers
> +                            button, null);
> +  // dispatch it on the target element
> +  target.dispatchEvent(mousedown);
> +}

Ah, there's actually a helper method for this that you can access like in clickElement. clickElement uses utils.synthesizeMouseAtCenter, but this utils object has many other helpful functions. utils is actual EventUtils: http://mxr.mozilla.org/mozilla/source/testing/mochitest/tests/SimpleTest/EventUtils.js and you can use its sendMouseEvent() function for this. There'll be less repeated code this way.

@@ +634,5 @@
> +  // viewport coordinates
> +  var clientX = Math.round(x(0)), clientY = Math.round(y(0));
> +  // Remember the coordinates
> +  var lastX = clientX, lastY = clientY;
> +  // remember the initial mousedown event

It doesn't seem like we keep track of the mousedown event. Can we get rid of this comment?

@@ +896,5 @@
>    }
>  }
>  
>  /**
> + * Function that perfrom a mouse tap

perform typo

@@ +1091,5 @@
>      i = 0;
>    }
> +  if (typeof isTap === "undefined") {
> +    isTap = false;
> +  }

In the case of segmented action chains, it is possible that someone will do:

actions.press(el).perform()
actions.release(el).perform()

In this case, we want to check check the last known touch event sent and see if it was a touchstart, and if so, then we may still need to check if it is a tap.

We should check if touchId is not null, and if so, we should set lastTouch before going into the switch block.
Attachment #733593 - Flags: review?(mdas) → review-
Attached patch mouseEvent (obsolete) — Splinter Review
A couple of things to note here:
1. click event for mouse is automatically generated after mouseup by synthesizeMouse in EventUtils; therefore, we don't need to fire "click" event explicitly.
2. In the case of segmented action chains, we need an extra parameter to keep track of the type of last event because lastTouch doesn't contain the event type. I noticed the previous version of long press didn't support segmented action chains, so I edited that in this patch as well. 
3. This patch still includes the gestures outside the action chain object. Since those functions are currently documented, we can remove them all in future bugs. Feedback?
Attachment #733593 - Attachment is obsolete: true
Attachment #736045 - Flags: review?(mdas)
Blocks: 859356
Attached patch small fix (obsolete) — Splinter Review
Attachment #736045 - Attachment is obsolete: true
Attachment #736045 - Flags: review?(mdas)
Attachment #736518 - Flags: review?(mdas)
Comment on attachment 736518 [details] [diff] [review]
small fix

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

Works great, mind if you fix some small nits before I check it in?

::: testing/marionette/marionette-listener.js
@@ +638,5 @@
> +    var dt = time - startTime;
> +    let last = dt + EVENT_INTERVAL / 2 > duration;
> +    // if so, we move all the way
> +    if (last)
> +      dt = duration;

i don't think dt = duration does anything for us, can you get rid of these two lines?

@@ +1035,5 @@
>        emitTouchEvent('touchend', touch);
> +      if (isTap && mouseEvent) {
> +        emitMouseEvent(el.ownerDocument, 'mousemove', 1, 0, touch.clientX, touch.clientY);
> +        emitMouseEvent(el.ownerDocument, 'mousedown', 1, 0, touch.clientX, touch.clientY);
> +        emitMouseEvent(el.ownerDocument, 'mouseup', 1, 0, touch.clientX, touch.clientY);

can't we use mousetap here?

@@ +1119,5 @@
>        emitTouchEvent('touchend', touch);
> +      if (isTouchStart && mouseEvent) {
> +        emitMouseEvent(touch.target.ownerDocument, 'mousemove', 1, 0, touch.clientX, touch.clientY);
> +        emitMouseEvent(touch.target.ownerDocument, 'mousedown', 1, 0, touch.clientX, touch.clientY);
> +        emitMouseEvent(touch.target.ownerDocument, 'mouseup', 1, 0, touch.clientX, touch.clientY);

can we use mousetap here, too?
Attachment #736518 - Flags: review?(mdas) → review+
Attached patch small changes (obsolete) — Splinter Review
Attachment #736518 - Attachment is obsolete: true
Attachment #737633 - Flags: review?(mdas)
Comment on attachment 737633 [details] [diff] [review]
small changes

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

::: testing/marionette/marionette-listener.js
@@ +1115,5 @@
>        touch = lastTouch;
>        lastTouch = null;
>        emitTouchEvent('touchend', touch);
> +      if (isTouchStart && mouseEvent) {
> +        mousetap(touch.target, 3000, touch.clientX, touch.clientY, 1, 0, null);

Why 3000ms? Shouldn't it be something low, like 25 used in the other instances?
Attached patch changeDurationSplinter Review
All previous single_tap has 3000ms duration while double_tap has 25ms duration. This is a inconsistency. Changed all 3000ms duration into 25ms.
Attachment #737633 - Attachment is obsolete: true
Attachment #737633 - Flags: review?(mdas)
Attachment #738569 - Flags: review?(mdas)
Comment on attachment 738569 [details] [diff] [review]
changeDuration

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

looks good and works well, thanks!
Attachment #738569 - Flags: review?(mdas) → review+
Our merge tool is going to nuke the checkin-needed when this lands on m-c anyway, so just set it after :)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/cffcceab1b0d
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
We'll need to get this 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.