827783 - click events sent by marionette are discarded in gaia

Status: VERIFIED FIXED

(Remote Protocol :: Marionette, defect)

Description

[Florin Strugariu [:Bebe]]

Starting with the 2013-01-05 build the click events are discarded by Gaia.

This is because:
https://bugzilla.mozilla.org/show_bug.cgi?id=823619
https://github.com/mozilla-b2g/gaia/commit/d30c5b96a83b3bbd8aedf92be28e8319a20961b3#L16R2

Any real mouse events (non-synthetic events with isTrusted true) that are sent by gecko are captured and discarded.

This kills our ability to click on any element.

Comment 1

[David Flanagan [:djf]]

That's only for apps that use shared/js/mouse_event_shim.js, but that's still a lot of apps.

Is there any way you can set some other flag in marionette events? Then we could check for it in the shim.

Or, if marionette only sends mouse events and never sends touch events, we could modify the shim so that it only discards mouse events if it sees a touch event.

Or, you could modify your marionette tests to send touch events along with mouse events in the way that gecko does, so that the shim does the right thing. It would cancel your mouse events but synthesize its own based on the touch events.  That, really, would be the way to go here, if you can do it.

Comment 2

[Daniel Holbert [:dholbert]]

w/ up-to-date gaia, my mouseclicks have no effect on b2g desktop client. stephend suggested that's likely related to this bug.

Comment 3

[David Flanagan [:djf]]

Daniel,

Are you using a new build of b2g desktop?  If what you're seeing is the new mouse_event_shim.js thing, it tries to shut itself off when running on a platform with no touch events.

It used to be that on the deskop we reported that touch events were supported, but Chris Jones says he changed that, to avoid this problem.

You could edit apps/homescreen/index.html to comment out the mouse_event_shim.js script. If that makes your problem go away, then it is related to this bug.

Cc'ing Chris.

Comment 4

[Daniel Holbert [:dholbert]]

(Sorry, disregard comment 2 -- I updated my gecko tree & rebuilt, and now my desktop client accepts clicks again. not sure what was going on, but it's fixed now -- sorry to have added noise on this bug.)

Comment 5

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

It's really odd, but we're now seeing things working as expected; our most-recent run, http://qa-selenium.mv.mozilla.com:8080/view/B2G/job/b2g.unagi.gaia.master.ui/779/console (which uses today's Unagi engineering build: unagi_2013-01-08_eng.zip), had passes in its unit tests and its swipe-to-unlock tests, among others that make use of touch events.

I'm still not sure why we were seeing problems earlier, but as far as our Jenkins CI is concerned, we're not seeing anything - makes me wonder if we had some bad flashes or workspace/state issues.

Sorry for the trouble/attention!

Marking WFM because I can no longer reproduce this.

Bebe, next time, can you link to a logfile or attach output of the failures?  Thanks!

Comment 6

[Florin Strugariu [:Bebe]]

this is still reproducible:

This fails only for apps that contain: mouse_event_shim.js
https://github.com/mozilla-b2g/gaia/blob/master/apps/clock/index.html#L25

The system app (it contains the lockscreen view) doesn’t import mouse_event_shim.js so click is still working there.


Apps that received the new mouse events are: 
clock
communication/contracts
communication/dialer
fm
gallery
homescreen
music
settings
video

This are almost the test that fail on the CI.

There is no log file that I can add. The events are just dropped.
https://github.com/mozilla-b2g/gaia/commit/d30c5b96a83b3bbd8aedf92be28e8319a20961b3#L16R41


The fail output is different depending on the situation.
It depends what the click does:

Sometimes after the click on a icon the iframe is not found
http://qa-selenium.mv.mozilla.com:8080/view/B2G/job/b2g.unagi.gaia.nightly.ui/75/testReport/junit/%28root%29/TestLaunchApp/test_launch_app/

Other times the test fails not finding the correct data because we are not on the correct page:
http://qa-selenium.mv.mozilla.com:8080/view/B2G/job/b2g.unagi.gaia.nightly.ui/75/testReport/junit/%28root%29/TestClockSwitchClockType/test_clock_switch_clock_type/

All these fails have a previous click() that is not triggered

Comment 7

[Aaron Train [:aaronmt]]

Confirming that this is still reproducible for me too on 01/09.

Comment 8

[David Flanagan [:djf]]

The events being discarded is completely expected for apps using the shim.

The ideal fix is to change marionette to send touch events (or touch+mouse events if you want to be really accurate).  Note that the synthetic_gestures.js file currently has touch events turned off and sends only mouse events.  There is a boolean at the end of that file that you can flip to have things like the swipe() method send touches instead of mouse events.

If synthetic_gestures.js is where your events are coming from, I'd just try flipping that switch.  Bonus points for modifying the tap() method to send touch and mouse events instead of just touch.

I'm not taking this bug myself because I don't know where the relevant code is and I wouldn't know how to test the fixes.  But I hope this comment helps.

If that doesn't work, see if you can modify the mouse events to add something non-sensical, like shift key held down.  Since we don't have a shift key on the device we could use that as a flag in the shim... If the shift key is down, we let the mouse events go through as a special case for marionette.

Sound good?

Comment 9

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

Should be able to test this against test_all_items_present_new_alarm(), here: https://github.com/mozilla/gaia-ui-tests/blob/master/gaiatest/tests/clock/test_clock_all_items_present_new_alarm.py#L18, as an example of a test from comment 6 that's affected.

Comment 10

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

Malini put together bug 828747 and landed what we hoped to be a quick, temporary fix, but although it's fixed a few cases, substituting tap() for click() has proven to be unreliable.  (We tested multiple pulls against the clock + contacts app, with mixed results.)

Comment 11

[David Flanagan [:djf]]

I'm not at all certain that I understand what is going on here.  But I gather that you have landed http://hg.mozilla.org/integration/mozilla-inbound/rev/df1dd3151535 to turn on touch events for methods like tap() and swipe().

Hopefully that means that events are no longer being discarded like they were before.

But you still need to be able to reliably click on things.

Before you had a tap method that send mousedown mouseup click, and that failed in apps that use the shim because they discard mouse events and synthesize their own from touch events. 

Now your tap method is sending touchstart touchend and apps that use the shim should be getting the synthetic mouse events they need.  But now you've got the opposite problem: apps that don't use the shim don't get the mousedown mouseup click events they need because you're sending only touch events.

Here's what all mobile devices do for taps.  They send this sequence of events:

  touchstart touchend mousemove mousedown mouseup click

I didn't know that when I wrote the synthetic gestures library, but I think that's what you need.  So modify the tap() method so it does something like this:

  // Dispatch touchstart and touchend events over the specified target
  // and then invoke the then() callback.
  // x and y are the relative to the viewport.
  // If not specified then the center of the target is used. t is the
  // optional amount of time between touchstart and touchend event.
  function tap(target, then, x, y, t) {
    if (!SyntheticGestures.touchSupported) {
      console.warn('tap: touch events not supported; using mouse instead');
      return mousetap(target, then, x, y, t);
    }

    if (x == null)
      x = '50%';
    if (y == null)
      y = '50%';

    var c = coordinates(target, x, y);

    touch(target, t || 50, [c.x0, c.x0], [c.y0, c.y0], function() {
      mousetap(target, then, x, y, t);
    });
  }

This is completely untested, but I think something like that should send the touch events and then send the mouse events.  It may leave out the initial mousemove, though.

And actually, looking at the comment before mousetap, it looks like it might omit the click event.  I don't know where these events get injected, so I don't actually know if gecko will create a click for you. I'm guessing you may need to send that yourself, modifying the mousetap() method to something like this:

 function mousetap(target, then, x, y, t) {
    if (x == null)
      x = '50%';
    if (y == null)
      y = '50%';
    var c = coordinates(target, x, y);

    drag(target.ownerDocument, t || 50, [c.x0, c.x0], [c.y0, c.y0], function() {
      SendAClickEventSomehow();
      then();
    });
  }

I hope this helps. Sorry my gesture library isn't up to snuff!

Comment 12

[David Flanagan [:djf]]

(In reply to Stephen Donner [:stephend] from comment #10)
> Malini put together bug 828747 and landed what we hoped to be a quick,
> temporary fix, but although it's fixed a few cases, substituting tap() for
> click() has proven to be unreliable.  (We tested multiple pulls against the
> clock + contacts app, with mixed results.)

I don't know what click() is, since that is not part of synthetic_gestures.js.  But I do know (see the comment above) that tap() does not actually send a click() event. You should add that.  But you've got to send touch events too or the apps that use the shim will break.

Comment 13

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

btw, Bug 829377 has landed, making tap() commands do the right thing by sending out the touch/mouse/click events. The gaia tests should be updated to use the tap command in lieu of the click() call so we get correct behaviour

Comment 14

[Armen [:armenzg]]

Is this bug worksforme?

The b2g panda gaia ui tests are passing.

Comment 15

[Zac C (:zac)]

WORKSFORME would be a disrespect to the effort that's gone towards getting the suite working again after the Gaia change!

Lots of the tests that don't work after the new changes have been disabled and thus you don't run them if you run via the manifest file.

Some tests still work with the old click().

Comment 16

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

click() events were being 'discarded' by gaia since everyone moved to touch events. Since Bug 829377, we're using tap() in lieu of click() to get the right action. 

Click() might still work on some apps (apps that are using the mouse event shim will still see a click), but we shouldn't continue using it, since the apps are going forward with touch events. See Bug 829377 for the full discussion. 

Due to the switch in gaia, there may be more inconsistencies. Please raise bugs specific to each problem. I'm closing this one.

Comment 17

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

Verified FIXED -- we're through this, and have filed the follow-up bugs.