1121353 - The touch event which is fired by TabParent::InjectTouchEvent does not go through APZC.

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

In marionette, we simulate touch event by calling TabParent::InjectTouchEvent [1]. But after bug 920036 has been landed, those events don't go through APZC. Some Copy/Paste marionette tests rely on long press event which is fired by APZC. Once those events don't go through APZC, I didn't receive long press event anymore.

[1]: https://dxr.mozilla.org/mozilla-central/source/testing/marionette/marionette-listener.js#124

Comment 1

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Send touch events which are generated by InjectTouchEvent to APZC.

Comment 2

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Comment on attachment 8548697 [details] [diff] [review]
Send touch events which are generated by InjectTouchEvent to APZC.

Send events which are generated by InjectTouchEvent to APZC. kats, do you think this is right track to resolved this issue? Thanks.

Comment 3

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8548697 [details] [diff] [review]
Send touch events which are generated by InjectTouchEvent to APZC.

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

Hm, no I have a different approach in mind to solve this. The main problem with this approach is that going forward all input events will go through APZ first and this TsbParent hook won't exist any more. In particular it won't make sense once we move APZ off the main thread as the injected input events will have to be asynchronous.

Instead what I propose is to move the gonk nsWindow::DispatchInputViaAPZ into nsBaseWidget (which makes sense anyway so we can reuse it across platforms) and expose that function via the nsIWidget interface. Then we can add a method on DOMWindowUtils or something to invoke it. Since I backed out bug 920036 last night I can move the function when I reland it and then write a patch to expose it. Then we can update marionette to use the new API.

For now we will have to leave injectTouchEvent as is because there is a bit of production Gaia code that uses it and if we change it that will break.

Comment 4

[Kartikaya Gupta (email:[e-mail])]

Actually looking at the code much of this already exists. We just need to wire up the nsIWidget::SynthesizeNativeTouchPoint function for the gonk nsWindow and then you can use nsDOMWindowUtils::SendNativeTouchPoint to inject touch events.

Comment 5

[Kartikaya Gupta (email:[e-mail])]

Attachment: Implement synthesizeNativeTouchPoint on gonk (untested)

This is what I had in mind. With this applied on top of the patch from bug 920036 (which I had to temporarily back out) you should be able to use the sendNativeTouchPoint API [1] on nsIDOMWindowUtils in the root process to simulate touch events. Does that sound like a reasonable approach?

[1] http://mxr.mozilla.org/mozilla-central/source/dom/interfaces/base/nsIDOMWindowUtils.idl?rev=9fe58932c192#677

Comment 6

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Implement SynthesizeNativeTouchPoint in the gonk widget to allow injecting touch events into APZ. r=

Initialize mInputType after MultiTouchInput created.
kats, this patch works well after tiny modification. Do you have any suggested reviewer for this patch? Thanks.

Comment 7

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Use DOMWindowUtils::SendNativeTouchPoint for emitTouchEvent.

Use SendNativeTouchPoint so that our touch event from marionette could go through APZC.

Comment 8

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

ni kats for comment 6.

Comment 9

[Kartikaya Gupta (email:[e-mail])]

(In reply to Morris Tseng [:mtseng] from comment #6)
> Created attachment 8549466 [details] [diff] [review]
> Implement SynthesizeNativeTouchPoint in the gonk widget to allow injecting
> touch events into APZ. r=
> 
> Initialize mInputType after MultiTouchInput created.
> kats, this patch works well after tiny modification. Do you have any
> suggested reviewer for this patch? Thanks.

Great! I would suggest :mwu as the reviewer. I hope to reland bug 920036 today so you should be good to land this once that is in.

Comment 10

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8549468 [details] [diff] [review]
Use DOMWindowUtils::SendNativeTouchPoint for emitTouchEvent.

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

::: testing/marionette/marionette-listener.js
@@ +130,5 @@
> +    case 'touchcancel':
> +      typeForUtils = domWindowUtils.TOUCH_CANCEL;
> +      break;
> +    case 'touchmove':
> +      typeForUtils = domWindowUtils.TOUCH_HOVER;

btw, I think this one should TOUCH_CONTACT as well. I didn't implement TOUCH_HOVER in gonk and just sending a new TOUCH_CONTACT with the same identifier as before will be counted as a move.

Comment 11

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Implement SynthesizeNativeTouchPoint in the gonk widget to allow injecting touch events into APZ v2.

Fix typo.

Comment 12

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Use DOMWindowUtils::SendNativeTouchPoint for emitTouchEvent v2.

Use TOUCH_CONTACT for touchmove case.

Comment 13

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5b5ca50df54a

Comment 14

[David Burns :automatedtester]

the GIP tests are failing after doing a tap which makes this the likely cause. please can you check and fix and set me as reviewer again for marionette stuff when complete

Comment 15

[Michael Wu [:mwu]]

Comment on attachment 8550124 [details] [diff] [review]
Implement SynthesizeNativeTouchPoint in the gonk widget to allow injecting touch events into APZ v2.

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

::: widget/gonk/nsWindow.cpp
@@ +322,5 @@
> +                                     nsIntPoint aPointerScreenPoint,
> +                                     double aPointerPressure,
> +                                     uint32_t aPointerOrientation)
> +{
> +    static nsTArray<SingleTouchData> sActiveTouchPoints;

Store this in nsWindow rather than in a static. Also, why use an array of SingleTouchData? Can't you keep a MultiTouchInput around? Preferably allocated as needed since this is test only.

@@ +332,5 @@
> +    MultiTouchInput input;
> +    input.mInputType = MULTITOUCH_INPUT;
> +    if (aPointerState == TOUCH_CONTACT) {
> +        size_t i = 0;
> +        while (i < sActiveTouchPoints.Length()) {

It looks like a helper function to get a touch point idx by ID would help. I think we might have one already somewhere in widget/gonk.

Comment 16

[Kartikaya Gupta (email:[e-mail])]

(In reply to Michael Wu [:mwu] from comment #15)
> Store this in nsWindow rather than in a static. Also, why use an array of
> SingleTouchData? Can't you keep a MultiTouchInput around? Preferably
> allocated as needed since this is test only.

I didn't see the point of exposing this to the rest of nsWindow since it's test-only state which is why I made it static inside the function. But yeah, I can move it and make it a MultiTouchInput if you prefer.

> @@ +332,5 @@
> > +    MultiTouchInput input;
> > +    input.mInputType = MULTITOUCH_INPUT;
> > +    if (aPointerState == TOUCH_CONTACT) {
> > +        size_t i = 0;
> > +        while (i < sActiveTouchPoints.Length()) {
> 
> It looks like a helper function to get a touch point idx by ID would help. I
> think we might have one already somewhere in widget/gonk.

Will do.

Comment 17

[Kartikaya Gupta (email:[e-mail])]

Attachment: Implement synthesizeNativeTouchPoint on gonk (v2)

(Again, I've compiled this but haven't run it so Morris might have to tweak it if there's bugs).

Comment 18

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Use DOMWindowUtils::SendNativeTouchPoint for emitTouchEvent v3.

Fix b2g-desktop try fail.

Comment 19

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #17)
> Created attachment 8551432 [details] [diff] [review]
> Implement synthesizeNativeTouchPoint on gonk (v2)
> 
> (Again, I've compiled this but haven't run it so Morris might have to tweak
> it if there's bugs).

I tried it and it works well. Thanks!

Comment 20

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

New try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d78ca58edf90

Comment 21

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Implement SynthesizeNativeTouchPoint on gonk (v3).

Fix build fail.

Comment 22

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

new try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7e9922773ac2

Comment 23

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Comment on attachment 8551638 [details] [diff] [review]
Use DOMWindowUtils::SendNativeTouchPoint for emitTouchEvent v3.

Try is good. David, could you review it again? Thanks.

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b884c6f42e63
https://hg.mozilla.org/integration/mozilla-inbound/rev/81af81149408

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b884c6f42e63
https://hg.mozilla.org/mozilla-central/rev/81af81149408

Comment 26

[Kartikaya Gupta (email:[e-mail])]

Morris, are you interested in getting this uplifted to b2g37? I would like like at least the gecko patch uplifted, as it will simplify the uplift for bug 930939. I don't care too much about the marionette patch.

Comment 27

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Sure, I'll uplifted those patches.

Comment 28

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Comment on attachment 8551638 [details] [diff] [review]
Use DOMWindowUtils::SendNativeTouchPoint for emitTouchEvent v3.

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: Some marionette tests cannot run on real device.
Testing completed: Test on local and try on master as well
Risk to taking this patch (and alternatives if risky): low risk.
String or UUID changes made by this patch: none

Comment 29

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Comment on attachment 8551671 [details] [diff] [review]
Implement SynthesizeNativeTouchPoint on gonk (v3).

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: Some marionette tests cannot run on real device.
Testing completed: Test on local and try on master as well
Risk to taking this patch (and alternatives if risky): low risk.
String or UUID changes made by this patch: none

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/6e3e0d0a2b39
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/71806c915c92