Closed Bug 829566 Opened 7 years ago Closed 7 years ago

marionette.tap() is triggering both click & tap events for the same element in dialer app

Categories

(Testing :: Marionette, defect)

defect
Not set

Tracking

(blocking-b2g:-, firefox20 wontfix, firefox21 fixed, b2g18+ fixed)

RESOLVED FIXED
mozilla21
blocking-b2g -
Tracking Status
firefox20 --- wontfix
firefox21 --- fixed
b2g18 + fixed

People

(Reporter: Bebe, Assigned: mdas)

References

Details

Attachments

(1 file, 1 obsolete file)

After the 829377 fix we are getting duplicated events in the dialer app

We are taping on elements representing numbers on the keypad.

Each self.marionette.tap(key) triggers 2 key presses (dials the same number twice)

I suspect that this is because bug 829377 is triggering one click event and one tap event at the same time.

You can reproduce this using: https://github.com/bebef1987/gaia-ui-tests/blob/fix_dialer/gaiatest/tests/test_dialer.py
Summary: marionette is duplicating click & tap in dialer app → marionette.tap() is triggering both click & tap events for the same element in dialer app
If I am reading https://groups.google.com/forum/?fromgroups=#!topic/mozilla.dev.gaia/as_XclduM78 correct then this may be a bug in the app and not marionette since it does 

touchstart touchend mousemove mousedown mouseup click 

cc' djf for his thoughts on this
it's also probably related to bug 829377
Try sticking debugging statements in the mouse_event_shim.js in the discardEvent() function to see if e.isTrusted is set on the mouse events that marionette is sending.

If the events are not trusted, then then shim isn't discarding them.

Have you instrumented the tap() function (or the functions it calls) to be sure that the event sequence is the one you want?  Actually, you could add debugging event handlers in the shim in this case to see what events are coming out.

This is on a device, right?  If you're testing on a desktop build things might be different.
Also experiencing this in the music app now too.
(In reply to Zac C (:zac) from comment #4)
> Also experiencing this in the music app now too.

And in the dialer app, too -- we're not sure if this is an app issue, core Gaia, or Marionette -- nominating to get traction on this due to the time-frame.
blocking-b2g: --- → tef?
Vivien, who should investigate where the bug actually exists?

cjones, could this have anything to do with bug 823619?
Flags: needinfo?(21)
Possibly, but if so the test code needs to be fixed.
(In reply to Andrew Overholt [:overholt] from comment #6)
> Vivien, who should investigate where the bug actually exists?
> 
> cjones, could this have anything to do with bug 823619?

Yes, this is fallout from bug 823619. The end result that each tap() command in marionette should send out the following events:

touchstart touchend mousemove mousedown mouseup click 

Which marionette now does. It works with most apps that either employ the shim or use touch events only, but in some cases like this, we're seeing problems.

Like djf said, it maybe a problem in the shim or the way we're sending those events to make some pass through the shim.

I'm currently trying out djf's suggestions in Comment #3 to see if the shim is filtering out/emitting the right messages
It seems that all the events generated by marionette are not trusted. The shim lets the mouse events pass through, but if a touch event is untrusted, it will catch it and duplicate the action. So untrusted touch events are seen in the shim, and have their appropriate mouse events emitted. The untrusted mouse events we send over just pass through, so we get duplicate mouse events.

If we'll keep letting untrusted touch events through in the future, then it seems safe to remove the mouse event emission from marionette side, so we will only send touch events and rely on the shim for generating mouse events if that's what the app wants. Djf, thoughts?
There's still a problem with this approach. If you have an app that doesn't need any mouse events, and so, doesn't need/use the shim, then tap() events like I said in Comment #9 won't work for all dom elements.

For example, if you have link tags (<a>), then an untrusted touch event without any mouse events doesn't trigger a tap on that, since a touchdown/touchup on that element doesn't seem to register. We need the shim for just this case, which isn't an acceptable approach.

I wonder if firing a trusted touch event will create the needed mouse events for us. We may be able to send this touch event through the marionette content-listener, but I'll have to test it out.
blocking-b2g: tef? → -
tracking-b2g18: --- → +
Attached patch shim patch (obsolete) — Splinter Review
Since we can't send trusted events from marionette, then we're going to have to detect if we're using the shim or not, and send the appropriate tap() command.

This adds a check to the MarionetteTouchMixin to check for the existence of the shim. The tap function will either send just touch events, or the whole touch/mouse/click event chain depending on what MarionetteTouchMixin passes it.

I've tested this locally against all the tests I could run (I don't have a sim card) and I only got expected failures (like lockscreen failures). The dialer app works with this patch, and so do the other tests I used for other patches.
Attachment #702630 - Flags: review?(jgriffin)
Comment on attachment 702630 [details] [diff] [review]
shim patch

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

::: testing/marionette/client/marionette/marionette_touch.py
@@ +19,5 @@
>  
>      def tap(self, element):
> +        # we pass in touch/mouse/click events if mouse_event_shim.js isn't included in the gaia app
> +        # otherwise, we just send touch events. See Bug 829566
> +        shim = self.execute_script("return typeof window.wrappedJSObject.MouseEventShim === 'undefined';") 

nit: this variable name is a bit confusing, since it will be false if the shim *is* included.  Maybe we should change it to send_all or something similar?
Attachment #702630 - Flags: review?(jgriffin) → review+
Attached patch patch v1.1Splinter Review
changed variable name from comments and carried forward r+.

As usual, I forgot to bump the version number, so I added it in this patch.
Attachment #702630 - Attachment is obsolete: true
Attachment #702833 - Flags: review+
landed on m-i:
https://hg.mozilla.org/integration/mozilla-inbound/rev/43391698ce5e

And updated pypi package.

Will land on b2g18 once this lands on m-i
Whiteboard: [leave-open]
Flags: needinfo?(21)
I just tested this on my device this morning and it fixes the dialer test. Thanks Malini!
We're still seeing this, apparently, in https://github.com/mozilla/gaia-ui-tests/pull/279.
(In reply to Stephen Donner [:stephend] from comment #16)
> We're still seeing this, apparently, in
> https://github.com/mozilla/gaia-ui-tests/pull/279.

I was looking into it yesterday, but got derailed. I'll go back to it today.
I'm moving the ftu problem to a new bug, Bug 833061, since the original test_dialer.py problems were resolved.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Assignee: nobody → mdas
Whiteboard: [leave-open]
Target Milestone: --- → mozilla21
You need to log in before you can comment on or make changes to this bug.