Closed Bug 891882 Opened 11 years ago Closed 10 years ago

Several components rely on touch events are malfunctioning on B2G desktop (master)

Categories

(Firefox OS Graveyard :: Gaia::System::Lockscreen, defect)

Other
Gonk (Firefox OS)
defect
Not set
major

Tracking

(Not tracked)

VERIFIED FIXED
1.3 C2/1.4 S2(17jan)

People

(Reporter: zcampbell, Assigned: fabrice)

References

Details

Attachments

(3 files, 29 obsolete files)

17.91 KB, patch
vingtetun
: review+
Details | Diff | Splinter Review
2.06 KB, patch
ochameau
: review+
Details | Diff | Splinter Review
16.78 KB, patch
ochameau
: review+
Details | Diff | Splinter Review
Using the 10th of July master/b2g25 build, the lock screen cannot be unlocked.

It would seem to be related to the changeset made in this build:
https://bugzilla.mozilla.org/show_bug.cgi?id=889466

It is fine on device where touch events are sent properly.

BuildID: 20130710030203
SourceStamp: 04d8c309fe72
Kevin, mind taking a look?
Flags: needinfo?(kgrandon)
Doh - I guess B2G desktop doesn't support touch events? I suppose we could do something similar as to what we do in the browser to emulate touch events.
So I just downloaded the latest nightly B2G desktop (July 10th), and everything appears to be working fine for me with a DEBUG gaia build.

Zac - Can you provide some more information such as OS and commands you are using to generate and run the profile?

Also - can you try opening the profile up in a Firefox Browser and let me know if you are able to unlock the lock screen? Thanks!
Flags: needinfo?(kgrandon) → needinfo?(zcampbell)
Flags: needinfo?(dave.hunt)
I am able to replicate this by performing the following steps:

1. Download mozilla-central B2G desktop from https://ftp.mozilla.org/pub/mozilla.org/b2g/nightly/2013-07-10-03-02-03-mozilla-central/b2g-25.0a1.multi.mac64.dmg (I have Mac, but I believe Zac was replicating this on Linux, and we've seen it on Travis, which is also on Linux)
2. Launch the B2G desktop build using: /path/to/B2G.app/Contents/MacOS/b2g (to use the bundled profile)
3. Navigate through the first time usage app, and dismiss.
4. Simulate pressing the power button (see https://developer.mozilla.org/en-US/docs/Mozilla/Firefox_OS/Using_the_B2G_desktop_client#Usage_tips) to lock and power off screen.
5. Repeat 4 to power on screen and show lock screen.
6. Attempt to drag the unlock 'handle' up.

Expected: Able to drag bar up and click unlock button to unlock the simulator.

Actual: Nothing happens.
Flags: needinfo?(zcampbell)
Flags: needinfo?(dave.hunt)
Thanks for the info Dave. It appears that the problem might be to the way that the bundled profile is generated. I'm not able to reproduce this with a manually generated gaia DEBUG profile, and I will try the bundled profile tomorrow.

I'll need to look into this a bit more to figure out how we can include the proper profile. In the meantime I would not be opposed to backing out the lockscreen patch that caused this.
The profile is being generated via make, not via DEBUG=1 make. That's probably the case. My issue also was there when running without DEBUG=1 (I think debug=1 never worked in B2G desktop so that's why I always run without).
Strange, DEBUG profiles work fine for me, and I do believe we should be using DEBUG profiles for B2G desktop.

Are DEBUG profiles still broken for you today?
shared folder has something like mouse_event_shim.js Perhaps it can be of some use here?
Yeah with latest Gaia and latest B2G desktop everything is back to normal. However we need to fix profile generation for the bundled gaia.

It might be useful to update docs if breaking APIs... Did it now for B2G desktop: https://developer.mozilla.org/en-US/docs/Mozilla/Firefox_OS/Using_the_B2G_desktop_client.
Ok, so I believe the fix we're going to do for this is to ship the bundled profile with a DESKTOP=1 build of gaia. That should include the necessary extensions for touch events to work properly.
Attachment #777343 - Flags: review?(jhford)
This bug also affects b2g-25.0a1.multi.linux-x86_64.tar.bz2 .
`DESKTOP=1 make profile` just flat out fails for me, on master.

kochbuch:gaia ahecht$ DESKTOP=1 make profile 
make: *** No rule to make target `profile'.  Stop.
Don't specify profile. `DESKTOP=1 make` is enough
Given how ubiquitous documentation about `make profile` is, I'd rather have that work, or at least produce a useful error message.
This patch seems to affect only WINNT.  If we're seeing this problem on desktop, what's the proper workaround?
Right, I need to refactor and test this script better =/ It should just need a DESKTOP=1 flag for either environment.

I guess I'm not too sure on how to run this Makefile - can anyone point me to some documentation on doing so?
sorry, s/desktop/mac/

Hacking the makefile so it sends DESKTOP=1 for !winnt doesn't change anything for me.

> I guess I'm not too sure on how to run this Makefile - can anyone point me to some 
> documentation on doing so?

I am also confused, as b2g/gaia does not appear in my objdir.
tested locally on mac now, specifying DESKTOP=1 does fix the unlock problem. Caveat is that the profile is in profile-debug instead of profile, so yet more things that work differently than documented everywhere.
John, can you review and land, or fix this correctly?
Assignee: nobody → jhford
Comment on attachment 777343 [details] [diff] [review]
Patch to use DESKTOP=1 in b2g desktop builds

I can run b2g desktop with a 'make' profile, but not a 'DESKTOP=1 make'. Because of this, I think something is wrong with the gaia build. Needs some more investigation.

John - I'll sit down with you once we have this figured out and we can hammer out the script.
Attachment #777343 - Attachment is obsolete: true
Attachment #777343 - Flags: review?(jhford)
excellent.

(one possible workaround for people hitting this is to run inside of FF.  Not the same thing, but depending on what you're doing, you might be able to get unstuck.  https://developer.mozilla.org/en-US/docs/Mozilla/Firefox_OS/Using_Gaia_in_Firefox)
I am still having lock screen touch event issues as of 08/26/2013 on my mac.

http://ftp.mozilla.org/pub/mozilla.org/b2g/nightly/latest-mozilla-central/b2g-26.0a1.multi.mac64.dmg

With both the bundled profile, and with gaia master (with & without DESKTOP=1, DEBUG=1) I get the following error, and cannot unlock the simulation once it's locked:

************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "'[JavaScript Error: "this.oncancel is not a function" {file: "app://system.gaiamobile.org/js/list_menu.js" line: 147}]' when calling method: [nsIDOMEventListener::handleEvent]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "JS frame :: app://system.gaiamobile.org/js/screen_manager.js :: scm_fireScreenChangeEvent :: line 509"  data: yes]
************************************************************

Is this a new issue, or was this one never resolved?
Not new, and shows up also on device.
Assigning to Kevin to figure out what's going on, per comment 22.
Assignee: jhford → kgrandon
http://ftp.mozilla.org/pub/mozilla.org/b2g/nightly/latest-mozilla-central/b2g-27.0a1.multi.win32.zip

I cannot unlock either - just running b2g on Windows 7 - Home/End, PgUp and PgDown keys work.
I haven't had the time to look into this recently, and not sure I'm the best person for this. Is there an owner of the "b2g desktop environment"? If so - we should probably talk to them about this.
Assignee: kgrandon → nobody
Same problem and I need it to reproduce a bug...

Can the lock screen be bypassed by some changes in the code?  So that pressing the home or end will go directly in?
(In reply to Honza Bambas (:mayhemer) from comment #30)
> Same problem and I need it to reproduce a bug...
> 
> Can the lock screen be bypassed by some changes in the code?  So that
> pressing the home or end will go directly in?

So my workaround is:
It works for a regular clean profile (not DEBUG=1 make)
So remove old profile folder, do a new make.
Start b2g-desktop and it should show you the first-time-usage (FTU) app.
Finish FTU, go into settings app and disable the lock screen in the 'screen-lock' option.
Now you should be able to use your profile after a restart.
This bug will fail the

1. Lockscreen
2. UtilityTray (can't pull it down from the status bar)
3. Keyboard (can't swipe out)
I'm going to make a test page to isolate the symptom, and to see what touch behavior and handling method  would reproduce this bug.
I wrote an test application to handle touchstart, touchmove and touchend, and it works on FirefoxNightly. However, on B2G desktop, it was broken.

If you want to test it on your own local machine, here is the branch:

https://github.com/snowmantw/gaia/tree/issue891882

You'll find an "issue891882" app under the "test_apps". Then, you need to modify the "/build/apps-production.list" to include it in the production profile and make Gaia again. I've tried several times to only specify the app in the file but it didn't work, so I put the line "test_apps/*" in it to include all test apps.

After that, you should replace the "/b2g/gaia/profile" with the profile to let "/b2g/bin/b2g" depends on it, or you can execute "/b2g/bin/b2g-bin" with "-profile" to load the profile. With all these steps done you can pass the FTU and launch the "issue891882" app to see it works on FirefoxNightly but not on B2G desktop.

BTW, this demo and other malfunctioning components shows this is not only a lockscreen problem. So I'll change the title.
Summary: Cannot unlock on desktop B2G (master) → Several components rely on touch events are malfunctioning on desktop B2G (master)
Currently the main problem is that FirefoxNightly will trigger touch events, but B2G desktop *will not*. I've asked someone and one opinion is that these two tools should have the same behavior, but apparently they don't. So I leave a needinfo to ask who may know some details, and to see how can we solve this (port touch event mechanism to B2G desktop ?).
Flags: needinfo?(bugs)
Summary: Several components rely on touch events are malfunctioning on desktop B2G (master) → Several components rely on touch events are malfunctioning on B2G desktop (master)
Right now we don't have touch events in B2G desktop. We solve this in Firefox Nightly by including the touch event shim which has now been moved into gecko.

In bug 925199 this has been moved to /toolkit/ which should allow us to include it in b2g desktop fairly easily.

I'm going to do a NI? for paul as I think he was thinking about this lately and may have more details.
Depends on: 925199
Flags: needinfo?(paul)
(I don't know about that touch event shim)
(In reply to Olli Pettay [:smaug] from comment #37)
> (I don't know about that touch event shim)

So I cancel the NI. Thanks Olli.
Flags: needinfo?(bugs)
Attached patch patch (obsolete) — Splinter Review
Let's unlock the lockscreen ;)
Assignee: nobody → fabrice
Attachment #824694 - Flags: review?(21)
Comment on attachment 824694 [details] [diff] [review]
patch

That doesn't work oop :(
Attachment #824694 - Flags: review?(21)
Blocks: 914584
Attached patch touch-event-desktop.patch (obsolete) — Splinter Review
This patch works in oop desktop now. I'll push to try and to pine to check tests.
Attachment #824694 - Attachment is obsolete: true
Attachment #827721 - Flags: review?(21)
Comment on attachment 827721 [details] [diff] [review]
touch-event-desktop.patch

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

::: b2g/app/b2g.js
@@ -288,5 @@
>  pref("image.mem.max_decoded_image_kb", 30000); /* 30MB seems reasonable */
>  pref("image.onload.decode.limit", 24); /* don't decode more than 24 images eagerly */
>  
>  // XXX this isn't a good check for "are touch events supported", but
>  // we don't really have a better one at the moment.

I feel like you can remove the comment as well.

::: b2g/chrome/content/shell.js
@@ +648,5 @@
> +          }
> +      };
> +      let touchEventHandler = new TouchEventHandler(scope);
> +      touchEventHandler.start();
> +#endif

nit: add a line break

::: toolkit/devtools/touch-events.js
@@ +166,5 @@
> +                                [1], [1],                 // rx, ry
> +                                [0], [0],                 // rotation, force
> +                                1);                       // count
> +        return;
> +      }

I think it worth adding a comment for our devtools/simulator friends that does hack on this code as well.
Attachment #827721 - Flags: review?(21) → review+
https://hg.mozilla.org/integration/b2g-inbound/rev/3b0468c92157

Note to whom it may concern: this will break tests/python/gaia-ui-tests/gaiatest/tests/functional/lockscreen/manifest.ini until I can land https://github.com/mozilla-b2g/gaia/pull/13438 but we need the new m-c to get travis happy.
(In reply to Wes Kocher (:KWierso) from comment #44)
> Backed out in
> https://hg.mozilla.org/integration/b2g-inbound/rev/ed53f428d0f2 for breaking
> gaia-ui-test
> https://tbpl.mozilla.org/php/getParsedLog.php?id=30232719&tree=B2g-Inbound

Next time can you ping me or read the commit comment? That's an expected failure.
The tree has now been burning for almost a day (if you count from the first landing), which is really less than ideal - particularly given the sheriffs had no advance heads up about this / were not asked for the best approach to do the simultaneous landing.

The sheriffs do not merge burning trees, so we're now in a stalemate where the gaia landing is waiting on a green travis run, but that's not going to occur since the changes need to be on mozilla-central first. To make things worse, even if we do merge the burning tree (on promise that it should hopefully be green later, even though we have no guarantee that the gecko+gaia landings contain no mistakes), once the gaia changes land, they'll initially only auto-sync to b2g-inbound, and then only on the next merge to mozilla-central will we not have two burning trees confusing everyone.

We should:
1) Consider making the travis jobs use b2g-inbound, not mozilla-central.
2) Use a better workflow for making simultaneous gecko and gaia landings, eg either:
   a) Land the gaia change even if the travis run isn't green.
   b) First make a gaia change to disable the tests (or make them feature-detect so they are backwards-compatible), then once that's on mozilla-central, land the gecko change on b2g-inbound (since the travis run will be green), then once that is on mozilla-central land another gaia change to remove the support for the previous API/...

My preference would be for (2a). (This is how some other landings have already been doing it)

In the meantime, this is backed out:
remote:   https://hg.mozilla.org/integration/b2g-inbound/rev/37f9b4003991

Please do not reland without speaking to the sheriffs first, unless doing either (2a), (2b) or we've discussed another alternative in this bug.
(In reply to Ed Morley [:edmorley UTC+1] from comment #47)
> My preference would be for (2a). (This is how some other landings have
> already been doing it)

Sorry, (2b) even.
I like 2b also.
Use syntax:
fail-if = device == "desktop"

so as not to bust the on-device testing. 
this will completely exclude it from Travis. (It is already excluded from TBPL).
(In reply to Zac C (:zac) from comment #50)
> Use syntax:
> fail-if = device == "desktop"
> 
> so as not to bust the on-device testing. 
> this will completely exclude it from Travis. (It is already excluded from
> TBPL).

This patch actually makes the test pass, so I can't do that. I'm disabling the test for now (see https://github.com/mozilla-b2g/gaia/pull/13472)
Pardon me fabrice I meant `skip-if = device == "desktop"`
apologies!
This patch breaks the keyboard tests in UI tests app. I can't focus the text area but if I click the text input under it, the text area gets focus.
(In reply to Kan-Ru Chen [:kanru] from comment #56)
> This patch breaks the keyboard tests in UI tests app. I can't focus the text
> area but if I click the text input under it, the text area gets focus.

That was backed out...
(In reply to Fabrice Desré [:fabrice] from comment #57)
> (In reply to Kan-Ru Chen [:kanru] from comment #56)
> > This patch breaks the keyboard tests in UI tests app. I can't focus the text
> > area but if I click the text input under it, the text area gets focus.
> 
> That was backed out...

Yeah, I applied locally yesterday. On b2g-desktop.
There are indeed no drag events working. Mac and Linux here.

I would also like to add that I have to use debug profiles with the b2g desktop client for it to work.
I use a hacked gaia, so I'm not sure what the problem is, but the normal profile will freeze on boot screen.

Anyway my point is, I would appreciate that this fix would then also work for debug profiles.
debug profiles have a shim bundled in an extensions iirc. This patch will makes that useless, but we need to make sure that works oop too.
(In reply to Kan-Ru Chen [:kanru] from comment #56)
> This patch breaks the keyboard tests in UI tests app. I can't focus the text
> area but if I click the text input under it, the text area gets focus.

Any idea what's wrong here?
Flags: needinfo?(paul)
(In reply to Paul Rouget [:paul] from comment #61)
> (In reply to Kan-Ru Chen [:kanru] from comment #56)
> > This patch breaks the keyboard tests in UI tests app. I can't focus the text
> > area but if I click the text input under it, the text area gets focus.
> 
> Any idea what's wrong here?

The view returned by https://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/touch-events.js#180 is wrong (at least in this case). I have a try run with that fixed the keyboard issues at https://tbpl.mozilla.org/?tree=Try&rev=22f27c863827 but there are still other failures.
Ping me if I can offer some help debugging test failures here, I'm keen to see this bug resolved!
Trying to build gaia on Linux x86, getting same problem (can't unlock the screen)
Attached patch touch.patch (obsolete) — Splinter Review
Alex, this is the latest patch I used, that fixes the keyboard tests at least when using the UI tests app manually like Kanru did.
My findings so far are that, since we enabled touch events via a pref,
marionette is now dispatching touch events instead of mouse events.
  dom.w3c_touch_events.enabled --enabled--> document.createTouch
http://mxr.mozilla.org/mozilla-central/source/testing/marionette/marionette-listener.js#881

It looks like Gaia unit tests are only run on desktop, right?
If that's the case, I would say these tests are broken on device and/or with touch events.
That may also be because of wrong touch events being dispatched by marionette-listener code.
(In reply to Alexandre Poirot (:ochameau) from comment #66)
> My findings so far are that, since we enabled touch events via a pref,
> marionette is now dispatching touch events instead of mouse events.
>   dom.w3c_touch_events.enabled --enabled--> document.createTouch
> http://mxr.mozilla.org/mozilla-central/source/testing/marionette/marionette-
> listener.js#881
> 
> It looks like Gaia unit tests are only run on desktop, right?
> If that's the case, I would say these tests are broken on device and/or with
> touch events.
> That may also be because of wrong touch events being dispatched by
> marionette-listener code.

We use Marionette on device to very good effect. There's not much we can't do on device. I guess in a sense we compromised desktopb2g to get good compatibility on device.

I'm going to ni? mdas as she knows this part of Marionette very well.
Flags: needinfo?(mdas)
Blocks: 920956
That was far from being obvious, but what happens is that marionette
is dispatching touch events as said in previous comment.
But has a bug, that only happens on desktop, just because screenX/screenY are equal to 0 on device.
The touch events coordinates ended up being wrong on desktop.

Let see if that fix all issues left.

https://tbpl.mozilla.org/?tree=Try&rev=835c0679d853
Attachment #827721 - Attachment is obsolete: true
Attachment #8339382 - Attachment is obsolete: true
Attached patch Fix marionette tap event (obsolete) — Splinter Review
Thanks to vivien's expertise on events, we fixed another failure in marionette.

I hope this is the good one:
https://tbpl.mozilla.org/?tree=Try&rev=f1ad4c685664
Attachment #8339990 - Attachment is obsolete: true
Marionette bits look good, and I tested it against my inari, but it seems like the new patch is still making desktop b2g Gu tests unhappy.
Flags: needinfo?(mdas)
Oh right, now that's because of DESKTOP_SHIMS=1 being set for Gu tests.
It register the touch-events twice...
Hum I'm clueless now. I'm no longer able to reproduce the failures now.

Could it be that marionette-listener.js is pulled from another revision/repo
than the one I push to try?

It really looks like my modifications made to this file aren't actually used!
Flags: needinfo?(zcampbell)
Alexandre, I'm pretty sure it uses the in-tree Marionette but I'll double check. 

Is there a way to download the build that TBPL used? Then we can use it manually, locally and see if it works just normally too?
Flags: needinfo?(zcampbell)
(In reply to Zac C (:zac) from comment #74)
> Alexandre, I'm pretty sure it uses the in-tree Marionette but I'll double
> check. 
Yeah try will use the marionette that's in-tree, so if you made changes to marionette it should pick them up. Looking at the output of the Gu log, it looks like it is using the in-tree marionette.
> 
> Is there a way to download the build that TBPL used? Then we can use it
> manually, locally and see if it works just normally too?

The appropriate build should be somewhere in ftp://ftp.mozilla.org/pub/firefox/try-builds/
(In reply to Zac C (:zac) from comment #74)
> Alexandre, I'm pretty sure it uses the in-tree Marionette but I'll double
> check. 
> 
> Is there a way to download the build that TBPL used? Then we can use it
> manually, locally and see if it works just normally too?

If you click on the "B", in the bottom panel you have a link to build directory.
The build is one of these tarball:
http://ftp.mozilla.org/pub/mozilla.org/b2g/try-builds/apoirot@mozilla.com-6ff688252ae1/try-linux64_gecko/
I've downloaded that Try build, ran it locally and the tests run/pass (unlike TBPL) and it also resolves the original bug from comment #0 when using the build manually.

I could not verify that this did not cause the device to regress.
Just pushed a rebase to ensure that it isn't related to buggy gaia changeset.
  https://tbpl.mozilla.org/?tree=Try&rev=8b8d60fd094b

The previous pushed changeset was refering to 087d109c83235fc157b2efac7ace6cc54a4c5aaa gaia changeset (via gaia.json in m-c)
But when using that revision locally, I wasn't even able to run test_cards_view_with_three_apps:
  AttributeError: 'TestCardsViewThreeApps' object has no attribute '_script_timeout'
New try that over-increase marionette timeout, to see if that's because of slow execution:
  https://tbpl.mozilla.org/?tree=Try&rev=fd802b5dada0
My last patch was just not working as expected, I'm even wondering why it was passing tests locally...
New try again:
  https://tbpl.mozilla.org/?tree=Try&rev=903e98a2f0b2
(It would only fail with DESKTOP_SHIMS=1)
Attachment #8340130 - Attachment is obsolete: true
I see that the Gu tests are failing due to a gaiatest installation problem. This is because the gaia version in your mozilla-central is a bit too old. A patch landed on Nov 29th that changes how Gu tests are run, so you'll need to rebase on top of tip mozilla-central to get Gu results.
Thanks mdas, that was it \o/

https://tbpl.mozilla.org/?tree=Try&rev=4b5d393ad7d4

Now we need to coordinate. I had to do something not really elegant in this patch, just to avoid patching gaia, mainly to ensure that it would work at the end. It would be better for me to first patch gaia, land the patch on gaia, and then try to land the gecko patch.

zac, you told me earlier that it would break travis. Can you please tell me more and how to coordinate?
Blocks: 946205
Depends on: 946231
Attached patch final patch (obsolete) — Splinter Review
Attachment #8341776 - Attachment is obsolete: true
Comment on attachment 8342404 [details] [diff] [review]
final patch

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

This patch depends on gaia bug 946231, which itself depends on gecko bug 946222.
So I'll wait for all these dependencies to land before repushing to try
as we can't specify a custom gaia to try.
Attachment #8342404 - Flags: review?(21)
Comment on attachment 8342404 [details] [diff] [review]
final patch

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

That's not gonna work in b2g oop afaict. Can we kill two birds with a stone please?
Attachment #8342404 - Flags: feedback-
Attached patch Fix OOP support (obsolete) — Splinter Review
OOP support is yet another thing. We first need to debunk b2g desktop support.
There is already enough complexity and moving parts here...

But here is a rebase of your original patch, with you as credit,
that still makes touch events work when OOP is enabled.
Comment on attachment 8342404 [details] [diff] [review]
final patch

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

I would like a specific file. That makes it easier to track what is desktop only. A review from a marionette peer will be nice as well.

::: b2g/chrome/content/shell.js
@@ +623,5 @@
> +                       .QueryInterface(Ci.nsIDocShell).chromeEventHandler;
> +    let touchEventHandler = new TouchEventHandler(frame);
> +    touchEventHandler.start();
> +#endif
> +

What about a specific file that is loaded by shell.html when we are in non-device mode? That will let us add other sugar functions for desktop only?

::: testing/marionette/marionette-listener.js
@@ +663,5 @@
>                      {log: elementManager.wrapValue(marionetteLogObj.getLogs())});
>      marionetteLogObj.clearLogs();
>      */
>      let domWindowUtils = curFrame.QueryInterface(Components.interfaces.nsIInterfaceRequestor).getInterface(Components.interfaces.nsIDOMWindowUtils);
> +    domWindowUtils.sendTouchEvent(type, [touch.identifier], [touch.clientX], [touch.clientY], [touch.radiusX], [touch.radiusY], [touch.rotationAngle], [touch.force], 1, 0);

I can't see what change has been made here?

@@ +697,5 @@
>   * Helper function that perform a mouse tap
>   */
>  function mousetap(doc, x, y) {
> +  // Ensure sending TOUCH source to offer a way to touch events shim on desktop
> +  // to ignore these events

This is not only that. We use this to ignore them on desktop but this is also the proper way to dispatch them. That's how they are dispatched by TabChild.cpp. See http://mxr.mozilla.org/mozilla-central/source/dom/ipc/TabChild.cpp#1629

@@ +700,5 @@
> +  // Ensure sending TOUCH source to offer a way to touch events shim on desktop
> +  // to ignore these events
> +  emitMouseEvent(doc, 'mousemove', x, y, null, null, Ci.nsIDOMMouseEvent.MOZ_SOURCE_TOUCH);
> +  emitMouseEvent(doc, 'mousedown', x, y, null, null, Ci.nsIDOMMouseEvent.MOZ_SOURCE_TOUCH);
> +  emitMouseEvent(doc, 'mouseup', x, y, null, null, Ci.nsIDOMMouseEvent.MOZ_SOURCE_TOUCH);

nit: Maybe we can have a little wrapper here that hides the 2 nulls ?

emitMouseFromTouchEvent(doc, 'mouse...', x, y);

@@ +1037,5 @@
>                         win,
>                         0,
>                         false, false, false, false,
>                         documentTouches,
>                         targetTouches,

I'm fine with the change here but I think it would make sense to have someone from marionette checking at those changes. For example the one where curFrame is replaced by doc makes sense to me but maybe there is some stuff I don't understand here.

::: toolkit/devtools/touch-events.js
@@ +140,5 @@
>        if (target && type) {
>          this.sendTouchEvent(evt, target, type);
>        }
>  
> +      if (evt.mozInputSource == Ci.nsIDOMMouseEvent.MOZ_SOURCE_TOUCH || !isSystemWindow) {

Worth a comment.
Attachment #8342404 - Flags: review?(21)
(In reply to Vivien Nicolas (:vingtetun) (:21) (RTO - Review Time Off until 09/12/13) from comment #88)
> Comment on attachment 8342404 [details] [diff] [review]
> ::: testing/marionette/marionette-listener.js
> @@ +663,5 @@
> >                      {log: elementManager.wrapValue(marionetteLogObj.getLogs())});
> >      marionetteLogObj.clearLogs();
> >      */
> >      let domWindowUtils = curFrame.QueryInterface(Components.interfaces.nsIInterfaceRequestor).getInterface(Components.interfaces.nsIDOMWindowUtils);
> > +    domWindowUtils.sendTouchEvent(type, [touch.identifier], [touch.clientX], [touch.clientY], [touch.radiusX], [touch.radiusY], [touch.rotationAngle], [touch.force], 1, 0);
> 
> I can't see what change has been made here?
> 

Instead of using screenX/Y, we are now using clientX/Y. touch event were having wrong coordinates...
Attached patch interdiff (obsolete) — Splinter Review
Attached patch Address vivien's review comments (obsolete) — Splinter Review
Attachment #8342404 - Attachment is obsolete: true
Comment on attachment 8343161 [details] [diff] [review]
Address vivien's review comments

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

mdas, Could you take a look at the marionette modification?

vivien, note that I provided an interdiff in attachment 8343158 [details] [diff] [review] (I omited desktop.js in the interdiff, but not in this patch)
Attachment #8343161 - Flags: review?(mdas)
Attachment #8343161 - Flags: review?(21)
Comment on attachment 8343161 [details] [diff] [review]
Address vivien's review comments

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

::: testing/marionette/marionette-listener.js
@@ +663,5 @@
>                      {log: elementManager.wrapValue(marionetteLogObj.getLogs())});
>      marionetteLogObj.clearLogs();
>      */
>      let domWindowUtils = curFrame.QueryInterface(Components.interfaces.nsIInterfaceRequestor).getInterface(Components.interfaces.nsIDOMWindowUtils);
> +    domWindowUtils.sendTouchEvent(type, [touch.identifier], [touch.clientX], [touch.clientY], [touch.radiusX], [touch.radiusY], [touch.rotationAngle], [touch.force], 1, 0);

This is still unclear to me what has changed here.
Attachment #8343161 - Flags: review?(21) → review+
(In reply to Vivien Nicolas (:vingtetun) (:21) (RTO - Review Time Off until 09/12/13) from comment #93)
> Comment on attachment 8343161 [details] [diff] [review]
> Address vivien's review comments
> 
> Review of attachment 8343161 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: testing/marionette/marionette-listener.js
> @@ +663,5 @@
> >                      {log: elementManager.wrapValue(marionetteLogObj.getLogs())});
> >      marionetteLogObj.clearLogs();
> >      */
> >      let domWindowUtils = curFrame.QueryInterface(Components.interfaces.nsIInterfaceRequestor).getInterface(Components.interfaces.nsIDOMWindowUtils);
> > +    domWindowUtils.sendTouchEvent(type, [touch.identifier], [touch.clientX], [touch.clientY], [touch.radiusX], [touch.radiusY], [touch.rotationAngle], [touch.force], 1, 0);
> 
> This is still unclear to me what has changed here.

-    domWindowUtils.sendTouchEvent(type, [touch.identifier], [touch.screenX], [touch.screenY], ...
+    domWindowUtils.sendTouchEvent(type, [touch.identifier], [touch.clientX], [touch.clientY], ...

Is it clearer with this diff? Or do you not see why it would change anything to change from screen to client?
(In reply to Alexandre Poirot (:ochameau) from comment #94)
> (In reply to Vivien Nicolas (:vingtetun) (:21) (RTO - Review Time Off until
> 09/12/13) from comment #93)
> > Comment on attachment 8343161 [details] [diff] [review]
> > Address vivien's review comments
> > 
> > Review of attachment 8343161 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: testing/marionette/marionette-listener.js
> > @@ +663,5 @@
> > >                      {log: elementManager.wrapValue(marionetteLogObj.getLogs())});
> > >      marionetteLogObj.clearLogs();
> > >      */
> > >      let domWindowUtils = curFrame.QueryInterface(Components.interfaces.nsIInterfaceRequestor).getInterface(Components.interfaces.nsIDOMWindowUtils);
> > > +    domWindowUtils.sendTouchEvent(type, [touch.identifier], [touch.clientX], [touch.clientY], [touch.radiusX], [touch.radiusY], [touch.rotationAngle], [touch.force], 1, 0);
> > 
> > This is still unclear to me what has changed here.
> 
> -    domWindowUtils.sendTouchEvent(type, [touch.identifier],
> [touch.screenX], [touch.screenY], ...
> +    domWindowUtils.sendTouchEvent(type, [touch.identifier],
> [touch.clientX], [touch.clientY], ...
> 
> Is it clearer with this diff? Or do you not see why it would change anything
> to change from screen to client?

screen/client! yeah :)
Comment on attachment 8343161 [details] [diff] [review]
Address vivien's review comments

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

Once this is addressed, is it possible to test this in try or is there still a dependent gaia component?

::: testing/marionette/marionette-listener.js
@@ +676,3 @@
>   *           elClientX and elClientY are the coordinates of the mouse relative to the viewport
>   */
> +function emitMouseEvent(doc, type, elClientX, elClientY, clickCount, button, inputSource) {

inputSource isn't used

@@ +692,5 @@
> +    // to offer a way for event listeners to differentiate
> +    // events being the result of a physical mouse action.
> +    // This is especially important for the touch event shim,
> +    // in order to prevent creating touch event for these fake mouse events.
> +    domUtils.sendMouseEvent(type, elClientX, elClientY, button || 0, clickCount || 1, 0, false, 0, Ci.nsIDOMMouseEvent.MOZ_SOURCE_TOUCH);

MOZ_SOURCE_TOUCH should not be used in all cases since it's not a fair assumption to make. Also, Marionette implements webdriver which has certain implications of what events get dispatched on desktop, and can't make them look like they were dispatched from a touchscreen, since by default, desktop & mouse is assumed.

To achieve this, what you can do is detect if we're in b2g, and if so, then send MOZ_SOURCE_TOUCH. I'd declare inputSource variable in marionette-listener, and when a newSession is created, you can check if it's a b2g environment, and if so, set it to MOZ_SOURCE_TOUCH. You can set a flag here http://mxr.mozilla.org/mozilla-central/source/testing/marionette/marionette-listener.js#188. If inputSource is set, then we pass it to sendMouseEvents, if not, let it fall back to default.
Attachment #8343161 - Flags: review?(mdas) → review-
Attached patch Tune inputSource story (obsolete) — Splinter Review
mdas, I've introduced a function as suggested by Vivien,
emitMouseEventFromTouch, to prevent modifying default behavior of emitMouseEvent.
I keep thinking we should always set inputSource to TOUCH in all current/existing marionette-listener.js usages. 
As far as I can tell, emitMouseEvent is only used from this file.
(if not, this patch now prevents modifying other potential usages)
But all the usages I can see are related to faking touch events via real touch or mouse equivalents.
We should set TOUCH source for all such events.

As highlighted by vivien, marionette code is trying to replicate what TabChild.cpp does:
  http://mxr.mozilla.org/mozilla-central/source/dom/ipc/TabChild.cpp
TOUCH source is set to all mouse/contextmenu events.

Regarding try run, I will push to try once I can land gaia patch from bug 946231
(the tree is currently closed)
Attachment #8343158 - Attachment is obsolete: true
Attachment #8343161 - Attachment is obsolete: true
Attachment #8344648 - Flags: review?(mdas)
Here is a try build that bumps gaia.json to include the gaia fix:
https://tbpl.mozilla.org/?tree=Try&rev=4e604c8bae78
(In reply to Alexandre Poirot (:ochameau) from comment #97)
> Created attachment 8344648 [details] [diff] [review]
> Tune inputSource story
> 
> mdas, I've introduced a function as suggested by Vivien,
> emitMouseEventFromTouch, to prevent modifying default behavior of
> emitMouseEvent.
> I keep thinking we should always set inputSource to TOUCH in all
> current/existing marionette-listener.js usages. 
> As far as I can tell, emitMouseEvent is only used from this file.
> (if not, this patch now prevents modifying other potential usages)
> But all the usages I can see are related to faking touch events via real
> touch or mouse equivalents.
> We should set TOUCH source for all such events.
> 

This codepath will be shared by both touch and mouse event dispatching code as soon as we implement gestures on desktop. The generateEvents function will be used by desktop Firefox as well as all b2g platforms, so I'd like to have the touch source used only if b2g is detected. It'll just help us out when we get there.

> As highlighted by vivien, marionette code is trying to replicate what
> TabChild.cpp does:
>   http://mxr.mozilla.org/mozilla-central/source/dom/ipc/TabChild.cpp
> TOUCH source is set to all mouse/contextmenu events.
> 
> Regarding try run, I will push to try once I can land gaia patch from bug
> 946231
> (the tree is currently closed)

I noticed this try run isn't running the Mn tests on desktop and that's because it's only running opt builds. Mn needs debug builds. Do you mind rerunning with -b do as your build platform options in try?
Mochitests that try to dispatch mouse events are failing because of the helper cancelling them,
here is another try to fix that:
https://tbpl.mozilla.org/?tree=Try&rev=e239302768b2
Attached patch gecko patch (obsolete) — Splinter Review
The last patch didn't quite capture what I had described, so I just quickly modified it in this patch. It is your original patch but with changes to marionette-listeners.js, and these changes allow us to use the default input source when we are not running in b2g, but if we're in b2g, we will use MOZ_TOUCH.

I'm running it in try now https://tbpl.mozilla.org/?tree=Try&rev=13282a0061de

Are these changes acceptable to you?
Attachment #8344648 - Attachment is obsolete: true
Attachment #8344648 - Flags: review?(mdas)
Attachment #8346643 - Flags: review?(poirot.alex)
Comment on attachment 8346643 [details] [diff] [review]
gecko patch

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

Yep, that looks good.

There is still an issue with mochitest, I'll base my next patch on this one.
Attachment #8346643 - Flags: review?(poirot.alex) → review+
So I was assuming this bug was tracking fact that it was impossible to unlick the lockscreen in b2g-desktiom however it seems to have regressed it that even DESKTOP_SHIMS doesnt work.

For now b2g desktop is the most reasonable dev env, is there a bug tracking the fact it cant be opened?
Attached patch interdiff (obsolete) — Splinter Review
Fix mochitests and blacklist unfixable one
Attached patch patch, fix mochitests failures (obsolete) — Splinter Review
Yet another new patch for this...
This time, to address mochitests failures.
The touch event helper modify mouse event behavior,
it is especially breaking some click events being used here and there in mochitests.
And I had to disable one mochitest on b2g desktop,
because it makes too much assertion on mouse events behavior.
The most problematic assertion is against the usage of event.setCapture in the helper...

https://tbpl.mozilla.org/?tree=Try&rev=0749c3591b12
Attachment #8346643 - Attachment is obsolete: true
Attached patch Disable another mochitest (obsolete) — Splinter Review
http://mxr.mozilla.org/mozilla-central/source/content/html/content/test/forms/test_input_range_mouse_and_touch_events.html
Is doing tests against input range with mouse and touch events,
mouse events fails due to the touch event helper.
So I disabled mouse test (and kept touch ones) on b2g.

https://tbpl.mozilla.org/?tree=Try&rev=2f160dd883d6
Attachment #8348106 - Attachment is obsolete: true
Attached patch fix typo (obsolete) — Splinter Review
Attachment #8348717 - Attachment is obsolete: true
Attached patch Disable another mochitest (obsolete) — Splinter Review
Attachment #8348781 - Attachment is obsolete: true
Comment on attachment 8349475 [details] [diff] [review]
Disable another mochitest

And try is finally green \o/

Can you take a look at the modification I made to the touch event helper?
There is an interdiff attached in the bug, it only miss tests disablings.
Here is the two main modifications:

1) Stop cancelling mouse events to stop breaking absolutely all tests waiting for mousedown/mouseup.
3) Stop cancelling all click events by cancelling only the on we want to cancel. (fireMouseEvent(mousedown+mousemove+mouseup, evt) correctly dispatch a new click event, but also introduce unexpected mousedown/mousemove/mouseup events!)

I manually tested this patch on b2g dekstop with gaia, running OOP (with the other dedicated patch), or running NON-OOP, and everything seems to work fine. 
If I listen for mousedown and click events in the system and homescreen apps, I get excepted event, no more, no less and in an order that looks fine: touchstart, mousedown, touchend, click. (No sign of duplicated event or misorder)
Attachment #8349475 - Flags: review?(21)
Attached patch rebased patch for OOP support (obsolete) — Splinter Review
Attachment #8342458 - Attachment is obsolete: true
Attachment #8350043 - Flags: review?(21)
(In reply to Alexandre Poirot (:ochameau) from comment #111)
> I manually tested this patch on b2g dekstop with gaia, running OOP (with the
> other dedicated patch), or running NON-OOP, and everything seems to work
> fine. 
> If I listen for mousedown and click events in the system and homescreen
> apps, I get excepted event, no more, no less and in an order that looks
> fine: touchstart, mousedown, touchend, click. (No sign of duplicated event
> or misorder)

Very cool! Can you check with the settings app also? This is where I was getting duplicated events when tapping the check boxes and the toggles.
Comment on attachment 8349475 [details] [diff] [review]
Disable another mochitest

As discussed offline, this will send events with an unexpected order for touch events. isTrusted may help you for the tests purpose.
Attachment #8349475 - Flags: review?(21)
(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #114)
> As discussed offline, this will send events with an unexpected order for
> touch events. isTrusted may help you for the tests purpose.
If using .isTrusted helps, one could just add the event listener using
.addEventListener and use the 4th parameter.
http://mxr.mozilla.org/mozilla-central/source/dom/webidl/EventTarget.webidl?rev=3e42977d6d77#22
https://tbpl.mozilla.org/?tree=Try&rev=d949db0c0729
Let's see what test fails if we only listen for trusted events.

This interdiff is from the last r+ patch, not from the last r- one,
and only cares about touch-events.js modifications (do not include marionette, not test disabling)
Attachment #8348089 - Attachment is obsolete: true
Attachment #8349475 - Attachment is obsolete: true
Blocks: 946155
(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #114)
> Comment on attachment 8349475 [details] [diff] [review]
> Disable another mochitest
> 
> As discussed offline, this will send events with an unexpected order for
> touch events. isTrusted may help you for the tests purpose.

That still makes some tests fail:
  https://tbpl.mozilla.org/?tree=Try&rev=8c7b489f148f

I'll continue looking at this.
Yet another way to prevent touch events helper to mess up with various mochitests.

DOMWindowUtils.sendMouseEvent forces MOUSE input source to events if UNKNOWN or none is given.
That doesn't help filtering events in the touch event helper...
https://tbpl.mozilla.org/?tree=Try&rev=caecb367d07a
Attached patch Workaround submit click event (obsolete) — Splinter Review
To address content/html/content/test/test_bug557087-4.html failures.
Attached patch squashed patch (obsolete) — Splinter Review
Hopefully that one is the good one!

Vivien, that patch is the final one to land,
it is a rebased-squashed patch.
You can find interdiffs from the last r+ patch
in all attachments it obsoletes.
Attachment #8350187 - Attachment is obsolete: true
Attachment #8357274 - Attachment is obsolete: true
Attachment #8357737 - Flags: review?(21)
Attached patch Tweaks DOMWindowUtils (obsolete) — Splinter Review
Stop settings mouse input source to all DOMWindowUtils events, unless explicitely asked.
Right now, we can't set inputSource to 0/unknown because of this,
and we end up always setting inputSource to MOUSE.

That's problematic for touch event helpers as we would like to distinguish
real user mouse event from generated ones.
Filtering for trusted isn't enough as some mochitests send trusted event through DOMWindowUtils.
Attachment #8356611 - Attachment is obsolete: true
Attachment #8357139 - Attachment is obsolete: true
Attachment #8357740 - Flags: review?(bugs)
Comment on attachment 8357740 [details] [diff] [review]
Tweaks DOMWindowUtils

Have you gone through all the places we use SendMouseEvent to 
see that they don't expect nsIDOMMouseEvent::MOZ_SOURCE_MOUSE?

I'd prefer if we just fixed b2g usage of the API.

To be able to set input source to UNKNOWN, make the nsIDOMWindowUtils::sendMouseEvent* to use  [optional_argc]. That way you can
check whether some explicit value was passed, and then not override the value.
Otherwise use nsIDOMMouseEvent::MOZ_SOURCE_MOUSE.
I think such change would be less risky.
Attachment #8357740 - Flags: review?(bugs) → review-
(In reply to Olli Pettay [:smaug] from comment #124)
> Comment on attachment 8357740 [details] [diff] [review]
> Tweaks DOMWindowUtils
> 
> Have you gone through all the places we use SendMouseEvent to 
> see that they don't expect nsIDOMMouseEvent::MOZ_SOURCE_MOUSE?
> 
> I'd prefer if we just fixed b2g usage of the API.
> 
> To be able to set input source to UNKNOWN, make the
> nsIDOMWindowUtils::sendMouseEvent* to use  [optional_argc]. That way you can
> check whether some explicit value was passed, and then not override the
> value.
> Otherwise use nsIDOMMouseEvent::MOZ_SOURCE_MOUSE.
> I think such change would be less risky.

Actually, I do not really care about ensuring that we can set it to unknown.
My main goal here is to easily identify real mouse event from faked one for tests.
Filtering by trusted event isn't enough. Filtering only inputSource=mouse
isn't enough because 99% of event send via DOMWindowUtils::SendMouseEvent
ends up with mouse input source.

So what you are suggesting here will still set all input source to mouse.
I do not need to ensure that input source can be unknown,
I would have liked it to be unknown unless explicitely asked.

I'm afraid that if you disagree with that, we would have to find yet another way to filters test events.
The default should be MOZ_SOURCE_MOUSE. That is what the API is about, to send mouse events, as
similarly as possible as if they were actually coming from OS.
(In reply to Alexandre Poirot (:ochameau) from comment #125)
> Actually, I do not really care about ensuring that we can set it to unknown.
> My main goal here is to easily identify real mouse event from faked one for
> tests.
So I don't quite understand the requirement. Why do you need to handle test events differently?
Originally sendMouseEvent (well the whole DOMWindowUtils) was for testing only
(that is the reason for http://mxr.mozilla.org/mozilla-central/source/dom/base/nsDOMWindowUtils.cpp?rev=f861bf92cb7d#718) but it has since been used often for other cases too.

Anyhow, the reason for sendMouseEvent is to mimic native mouse events as well as possible.

So, when you say "identify real mouse event from faked one", what do you actually mean?
(In reply to Olli Pettay [:smaug] from comment #127)
> (In reply to Alexandre Poirot (:ochameau) from comment #125)
> > Actually, I do not really care about ensuring that we can set it to unknown.
> > My main goal here is to easily identify real mouse event from faked one for
> > tests.
> So I don't quite understand the requirement. Why do you need to handle test
> events differently?
> Originally sendMouseEvent (well the whole DOMWindowUtils) was for testing
> only
> (that is the reason for
> http://mxr.mozilla.org/mozilla-central/source/dom/base/nsDOMWindowUtils.
> cpp?rev=f861bf92cb7d#718) but it has since been used often for other cases
> too.
> 
> Anyhow, the reason for sendMouseEvent is to mimic native mouse events as
> well as possible.

Nevermind this input source patch, I'll find other ways to make the touch event helper work with tests.

> 
> So, when you say "identify real mouse event from faked one", what do you
> actually mean?

In order to keep these tests running on b2g:
https://tbpl.mozilla.org/php/getParsedLog.php?id=32478215&tree=Try
We need to prevent the touch event helper to translate mouse events being sent by these tests.
The touch event helper will cancel and/or delay mouse events while translating them to touch ones.
That doesn't break only tests against very precise mouse events behavior,
but also tests that listen for click events for various other reasons.
Comment on attachment 8357737 [details] [diff] [review]
squashed patch

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

r+ with nit and the new chrome only property on Event.webidl.

::: testing/marionette/marionette-listener.js
@@ +696,2 @@
>      let win = doc.defaultView;
> +    let domUtils = win.QueryInterface(Components.interfaces.nsIInterfaceRequestor).getInterface(Components.interfaces.nsIDOMWindowUtils);

nit: Components.interfaces -> Ci

::: toolkit/devtools/touch-events.js
@@ +62,5 @@
>      },
>      handleEvent: function teh_handleEvent(evt) {
> +      // Ignore all but real mouse event coming from physical mouse
> +      // (especially ignore mouse event being dispatched from a touch event)
> +      if (evt.button || evt.mozInputSource != Ci.nsIDOMMouseEvent.MOZ_SOURCE_MOUSE) {

I have seen an alternate approach while sitting next to you. The alternate approach would be fine.
Attachment #8357737 - Flags: review?(21) → review+
Ok, forget this input source hack.
What do you think about adding a chrome-only flag to DOMEvents,
so that we flag all domwindowutils as being "synthesized" unless specified otherwise.
It then offers a way to the touch event helper (which is in priviledged JS)
to ignore test events.
Attachment #8357740 - Attachment is obsolete: true
Attachment #8359121 - Flags: feedback?(bugs)
Comment on attachment 8359121 [details] [diff] [review]
Offer a way to distinguish tests events and ignore them in the touch event helper.


>   return SendMouseEventCommon(aType, aX, aY, aButton, aClickCount, aModifiers,
>                               aIgnoreRootScrollFrame, aPressure,
>-                              aInputSourceArg, false, aPreventDefault);
>+                              aInputSourceArg, false, aPreventDefault,
>+                              aOptionalArgCount == 4 ? aIsSynthesized : true);
> }
aOptionalArgCount >= 4

>   PROFILER_LABEL("nsDOMWindowUtils", "SendMouseEventToWindow");
>   return SendMouseEventCommon(aType, aX, aY, aButton, aClickCount, aModifiers,
>                               aIgnoreRootScrollFrame, aPressure,
>-                              aInputSourceArg, true, nullptr);
>+                              aInputSourceArg, true, nullptr,
>+                              aOptionalArgCount == 4 ? aIsSynthesized : true);
ditto
(it is just forwards compatible that way)
Attachment #8359121 - Flags: feedback?(bugs) → feedback+
Comment on attachment 8359121 [details] [diff] [review]
Offer a way to distinguish tests events and ignore them in the touch event helper.

Update uuid of nsIDOMWindowUtils
Here is a better patch, with tests.
Do you think I should flag additional reviewer for this patch?

https://tbpl.mozilla.org/?tree=Try&rev=b2bc4d0dbeb3
Attachment #8359121 - Attachment is obsolete: true
Attachment #8359295 - Flags: review?(bugs)
New patch, now using isSynthesized DOMEvent attribute.
Attachment #8357737 - Attachment is obsolete: true
Comment on attachment 8359295 [details] [diff] [review]
DOMWindowUtils.sendMouseEvent dispatch events with isSynthesized chrome attribute set to true

This r+ should be enough.
Attachment #8359295 - Flags: review?(bugs) → review+
Hum sendMouseEvent throws NS_ERROR_FAILURE on Mac... but only opt builds!
And no assertion/message on debug builds, the test just pass.
What could possibly go wrong with mouse events on Mac ?!!
Do you mean in the tests?
Does it help if you use sendMouseEventToWindow?
Yes, sendMouseEvent test is throwing, whereas it doesn't if I run it manually in the jsconsole...
So I'd tend to think it is some kind of race.
Various tests use setTimeout(..., 0)/executeSoon before running test with sendMouseEvent.
Let see if that fix it:
  https://tbpl.mozilla.org/?tree=Try&rev=7c16609f6927
It fixed it! So here is a new patch, with an additional executeSoon after load event, before running tests.
And new try run to ensure it didn't broke other platforms...
  https://tbpl.mozilla.org/?tree=Try&rev=a6ff81b8b97c
Attachment #8359295 - Attachment is obsolete: true
Attachment #8359722 - Flags: review?(bugs)
Comment on attachment 8359297 [details] [diff] [review]
main patch, rebased again

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

I do not have interdiff this time, but it is quite simple.
I added the check in touch event helper handleEvent function,
and modified the responsive design test against touch event helper
to ensure that events being dispatched form this particular test
have `isSynthesized` set to false.

::: browser/devtools/responsivedesign/test/browser_responsiveui_touch.js
@@ +31,2 @@
>      is(div.style.transform, "", "touch didn't work");
> +    EventUtils.synthesizeMouse(div, x, y, {type: "mouseup", isSynthesized: false}, content);

--^

::: toolkit/devtools/touch-events.js
@@ +62,5 @@
>      },
>      handleEvent: function teh_handleEvent(evt) {
> +      // Ignore all but real mouse event coming from physical mouse
> +      // (especially ignore mouse event being dispatched from a touch event)
> +      if (evt.button || evt.mozInputSource != Ci.nsIDOMMouseEvent.MOZ_SOURCE_MOUSE || evt.isSynthesized) {

--^
Attachment #8359297 - Flags: review?(21)
Attachment #8359722 - Flags: review?(bugs) → review+
Attachment #8350043 - Attachment is obsolete: true
Attachment #8359831 - Flags: review+
Try looks green, got r+, I think we are finally good to land!
Keywords: checkin-needed
Rebase against master due to conflict in mochitest.ini
Attachment #8359722 - Attachment is obsolete: true
Attachment #8360282 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/f890a8991588
https://hg.mozilla.org/mozilla-central/rev/16494b084901
https://hg.mozilla.org/mozilla-central/rev/539b03915ad7
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.3 C2/1.4 S2(17jan)
FYI - Looks like travis is red now, but expected due to a fail-if. Currently trying to enable the test, and will land when green: https://github.com/mozilla-b2g/gaia/pull/15380
(In reply to Kevin Grandon :kgrandon from comment #147)
> FYI - Looks like travis is red now, but expected due to a fail-if. Currently
> trying to enable the test, and will land when green:
> https://github.com/mozilla-b2g/gaia/pull/15380

We got a green travis build and landed: https://github.com/mozilla-b2g/gaia/commit/e5ec47861823333ce04b195f7922cd771ed11b4f
To any community still following this bug, the latest desktopb2g on ftp.mozilla is now unlockable and some other features that did not work previously like dialer now work, too! Good luck.
(In reply to Zac C (:zac) from comment #149)
> To any community still following this bug, the latest desktopb2g on
> ftp.mozilla is now unlockable and some other features that did not work
> previously like dialer now work, too! Good luck.

Thanks Zac, Working fine with Mac OSx 10.8.5 and latest nightly!
Status: RESOLVED → VERIFIED
Depends on: 1071197
Blocks: 1111566
You need to log in before you can comment on or make changes to this bug.