Closed Bug 1121353 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: Panning and Zooming, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox36 --- wontfix
firefox37 --- wontfix
firefox38 --- fixed
b2g-v2.1 --- unaffected
b2g-v2.2 --- fixed
b2g-master --- fixed

People

(Reporter: mtseng, Assigned: mtseng)

References

Details

Attachments

(2 files, 7 obsolete files)

3.69 KB, patch
automatedtester
: review+
Details | Diff | Splinter Review
10.75 KB, patch
mwu
: review+
Details | Diff | Splinter Review
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
Assignee: nobody → mtseng
Status: NEW → ASSIGNED
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.
Attachment #8548697 - Flags: feedback?(bugmail.mozilla)
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.
Attachment #8548697 - Flags: feedback?(bugmail.mozilla) → feedback-
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.
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
Initialize mInputType after MultiTouchInput created.
kats, this patch works well after tiny modification. Do you have any suggested reviewer for this patch? Thanks.
Attachment #8548697 - Attachment is obsolete: true
Attachment #8549017 - Attachment is obsolete: true
Use SendNativeTouchPoint so that our touch event from marionette could go through APZC.
Attachment #8549468 - Flags: review?(dburns)
ni kats for comment 6.
Flags: needinfo?(bugmail.mozilla)
(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.
Flags: needinfo?(bugmail.mozilla)
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.
Use TOUCH_CONTACT for touchmove case.
Attachment #8549468 - Attachment is obsolete: true
Attachment #8549468 - Flags: review?(dburns)
Attachment #8550125 - Flags: review?(dburns)
Attachment #8550125 - Flags: review?(dburns)
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 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.
Attachment #8550124 - Flags: review?(mwu)
(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.
(Again, I've compiled this but haven't run it so Morris might have to tweak it if there's bugs).
Attachment #8550124 - Attachment is obsolete: true
Attachment #8551432 - Flags: review?(mwu)
Fix b2g-desktop try fail.
Attachment #8550125 - Attachment is obsolete: true
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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!
Fix build fail.
Attachment #8551432 - Attachment is obsolete: true
Attachment #8551432 - Flags: review?(mwu)
Attachment #8551671 - Flags: review?(mwu)
Attachment #8551671 - Flags: review?(mwu) → review+
Comment on attachment 8551638 [details] [diff] [review]
Use DOMWindowUtils::SendNativeTouchPoint for emitTouchEvent v3.

Try is good. David, could you review it again? Thanks.
Attachment #8551638 - Flags: review?(dburns)
Attachment #8551638 - Flags: review?(dburns) → review+
https://hg.mozilla.org/mozilla-central/rev/b884c6f42e63
https://hg.mozilla.org/mozilla-central/rev/81af81149408
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
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.
Flags: needinfo?(mtseng)
Sure, I'll uplifted those patches.
Flags: needinfo?(mtseng)
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
Attachment #8551638 - Flags: approval-mozilla-b2g37?
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
Attachment #8551671 - Flags: approval-mozilla-b2g37?
Attachment #8551638 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
Attachment #8551671 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
You need to log in before you can comment on or make changes to this bug.