Brad Lassey [:blassey] (use needinfo?) Reporter
	Description • 11 years ago

shadow bug for  Bug #789358

	ffos-browser
	Comment 1 • 11 years ago

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

	Dale Harvey (:daleharvey) Assignee
	Comment 2 • 11 years ago

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

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 3 • 11 years ago

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?

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 4 • 11 years ago

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.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 5 • 11 years ago

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.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 7 • 11 years ago

Dale, any progress on this?

	Dale Harvey (:daleharvey) Assignee
	Comment 8 • 11 years ago

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

	Ben Francis [:benfrancis]
	Comment 9 • 11 years ago

Setting status to assigned for the benefit of the 1.2 dashboard

	Dale Harvey (:daleharvey) Assignee
	Comment 10 • 11 years ago

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

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 12 • 11 years ago

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.

	Dale Harvey (:daleharvey) Assignee
	Comment 13 • 11 years ago

Addressed feedback comments

	Dale Harvey (:daleharvey) Assignee
	Comment 14 • 11 years ago

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

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 15 • 11 years ago

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.

	Dale Harvey (:daleharvey) Assignee
	Comment 16 • 11 years ago

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

	Dale Harvey (:daleharvey) Assignee
	Comment 17 • 11 years ago

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

	Dale Harvey (:daleharvey) Assignee
	Comment 18 • 11 years ago

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

	Dale Harvey (:daleharvey) Assignee
	Comment 19 • 11 years ago

Forgot I should resolve this until it hits central

	Ryan VanderMeulen [:RyanVM]
	Comment 20 • 11 years ago

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

	Naoki Hirata :nhirata (please use needinfo instead of cc)
	Comment 21 • 11 years ago

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