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

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect)

Description

[Florin Strugariu [:Bebe]]

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

Comment 1

[David Burns :automatedtester]

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

Comment 2

[David Burns :automatedtester]

it's also probably related to bug 829377

Comment 3

[David Flanagan [:djf]]

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.

Comment 4

[Zac C (:zac)]

Also experiencing this in the music app now too.

Comment 5

[Stephen Donner [:stephend] Not actively reading bugmail]

(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.

Comment 6

[Andrew Overholt [:overholt]]

Vivien, who should investigate where the bug actually exists?

cjones, could this have anything to do with bug 823619?

Comment 7

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Possibly, but if so the test code needs to be fixed.

Comment 8

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

(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

Comment 9

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

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?

Comment 10

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

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.

Comment 11

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

Attachment: shim patch

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.

Comment 12

[Jonathan Griffin (: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?

Comment 13

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

Attachment: patch v1.1

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.

Comment 14

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

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

Comment 15

[Bob Silverberg [:bsilverberg]]

I just tested this on my device this morning and it fixes the dialer test. Thanks Malini!

Comment 16

[Stephen Donner [:stephend] Not actively reading bugmail]

We're still seeing this, apparently, in https://github.com/mozilla/gaia-ui-tests/pull/279.

Comment 17

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/43391698ce5e

Comment 18

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

pushed to b2g-18:
https://hg.mozilla.org/releases/mozilla-b2g18/rev/14a79fca527b

Comment 19

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

(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.

Comment 20

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

I'm moving the ftu problem to a new bug, Bug 833061, since the original test_dialer.py problems were resolved.