900638 - Fix event-fluffing to work with APZC

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Browser, defect)

Description

[Brad Lassey [:blassey] (use needinfo?)]

shadow bug for  Bug #789358

Comment 1

[ffos-browser]

A Pivotal Tracker story has been created for this Bug: http://www.pivotaltracker.com/story/show/54480278

Comment 2

[Dale Harvey (:daleharvey)]

On testing the event fluffing that has landed on central and is working great for the rest of the system does not work for web content inside the browser, debugging

Comment 3

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

I should be able to help you with that. In light of this bug I think there's some changes we need to do in the APZC code and maybe out some of the gesture detection. Let me quickly summarize what the current input event flow is in the browser:

1) Events come in to widget/gonk/nsAppShell.cpp and are dispatched via nsWindow into gecko
2) After working its way through the nsEventStateManager, the event arrives at TabParent (via nsEventStateManager::DispatchCrossProcessEvent)
3) TabParent::SendRealXXXEvent makes a copy of the event, and calls MaybeForwardEventToRenderFrame with both the original and the copy
4) The MaybeForwardEventToRenderFrame sends both events to the APZC. The APZC uses the original to drive pan/zoom animations, and updates the copy so that it unapplies any async transforms. This effectively maps the input from what the user sees on the screen to where gecko thinks the elements are
5) Coming back to TabChild::SendRealXXXEvent, the modified event copy is then send to the child process. The child process basically acknowledges the event and does nothing further with it.
6) Meanwhile, the APZC also runs gesture detection code on the original events from step 4, to detect things like taps, pinches, and so on. Some of the gestures detected (tap, long tap, and double tap) are sent via the GeckoContentController interface back to RenderFrameParent and TabParent, which passes it on to the child process (TabChild::RecvHandleSingleTap) which then does stuff with it. I don't believe any event fluffing takes place here.

This is a pretty complicated flow, and what I would like to see changed is to move some of the gesture detection out of the APZC code and into the TabChild code, to be run at step 5. The APZC will still need to do some gesture detection, but only for gestures that actually affect panning/zooming (i.e. pinches and flings) but for all other gestures I would like to move the code out. That will get rid of step 6 which is causing me much grief at the moment and will also allow the event fluffing to happen before gesture detection, which might make things simpler for you.

Another change I would like to do is get rid of step 2, so that the widget/gonk code sends the events directly to the APZC, which will need to do various transformations on the input events before it can properly decide which process needs to get the input events. That's outside the scope of this bug though and should be doable independently.

Does that make sense or help at all?

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Well, I'd assume E10s-Firefox needs 2) so that chrome JS can handle events.
Though, we do have that the ugly-shortcut for some touch events to bypass chrome DOM dispatch in b2g.

Comment 5

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

You're right, what I was thinking was to move step 2 to later in the process. That is, the APZC would get the raw screen-coord event, and do the untransform before it gets to nsEventStateManager. More importantly, it would be able to send input events directed at content directly to the content process without going through nsEventStateManager if necessary, which will get rid of that ugly shortcut code entirely.

Comment 7

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

Dale, any progress on this?

Comment 8

[Dale Harvey (:daleharvey)]

Its going but going slowly, every time I get interrupted it takes a while for me to get back into it, will be back on it tomorrow and hopefully get some better progress

Comment 9

[Ben Francis [:benfrancis]]

Setting status to assigned for the benefit of the 1.2 dashboard

Comment 10

[Dale Harvey (:daleharvey)]

So the problem was actually much simpler (after many false starts)

Within azpc frames the events are being reported with MOZ_SOURCE_MOUSE as the inputSource, this causes the check @ https://github.com/mozilla/mozilla-central/blob/master/layout/base/PositionedEventTargeting.cpp#L319 to fail, commenting out this check causes fluffing to work inside the browser

Comment 11

[Dale Harvey (:daleharvey)]

Attachment: Specify input source when generating mouse events

Comment 12

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

Comment on attachment 802449 [details] [diff] [review]
Specify input source when generating mouse events

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

This will work ok, but you don't really need most of the changes here. You can get rid of the changes to nsFrameLoader.cpp, PBrowser.ipdl, and TabParent.*. Also remove the change to RecvMouseEvent in TabChild.*, and instead make RecvHandleSingleTap and so on call DispatchMouseEvent directly instead of going through RecvMouseEvent. I never really understood why all things like RecvHandleSingleTap went through RecvMouseEvent anyway - it's misleading because the Recv implies that it's being called via ipdl when in fact it is not. Does that make sense? If you prefer to do that in another bug that's fine too but I just feel it's a bit unnecessary to add this input source to the ipdl.

Comment 13

[Dale Harvey (:daleharvey)]

Attachment: Bug 900638 - Specify input source when generating mouse events

Addressed feedback comments

Comment 14

[Dale Harvey (:daleharvey)]

Cheers that makes for a much cleaner patch, was confused at why the events went over ipdl but figured there was a reason, will push this to try when it reopens, with manual testing its working though

Comment 15

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

Comment on attachment 802622 [details] [diff] [review]
Bug 900638 - Specify input source when generating mouse events

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

Much better, thanks!

::: dom/ipc/TabChild.cpp
@@ +1890,5 @@
>                                               2 /* Right button */,
>                                               1 /* Click count */,
>                                               0 /* Modifiers */,
> +                                             false /* Ignore root scroll frame */,
> +                                             nsIDOMMouseEvent::MOZ_SOURCE_UNKNOWN);

Your original patch had this one as MOZ_SOURCE_TOUCH which I think is more correct. From the looks of it this code path is run based on touch input, but when APZC is disabled. So it should still be fluffed, I think. I'm fine with this either way though because this shouldn't affect behaviour in the browser content area where APZC is turned on.

Comment 16

[Dale Harvey (:daleharvey)]

Attachment: Bug 900638 - Specify input source when generating mouse events

Switched UNKNOWN source to TOUCH for context menu, carrying r+

Comment 17

[Dale Harvey (:daleharvey)]

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=929f9cf1350a

Comment 18

[Dale Harvey (:daleharvey)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/42a4dec61095

Comment 19

[Dale Harvey (:daleharvey)]

Forgot I should resolve this until it hits central

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/42a4dec61095

Comment 21

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Note to QA : This won't be in today's nightly build from releng, it will be in tomorrow's build.