Closed Bug 900638 Opened 6 years ago Closed 6 years ago

Fix event-fluffing to work with APZC

Categories

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

defect
Not set

Tracking

(blocking-b2g:koi+, b2g18 affected, b2g18-v1.0.1 affected)

RESOLVED FIXED
1.2 FC (16sep)
blocking-b2g koi+
Tracking Status
b2g18 --- affected
b2g18-v1.0.1 --- affected

People

(Reporter: blassey, Assigned: daleharvey)

References

Details

(Whiteboard: [sprintready][UCID: Browser1, FT:Browser, KOI:P1])

Attachments

(1 file, 2 obsolete files)

shadow bug for  Bug #789358
A Pivotal Tracker story has been created for this Bug: http://www.pivotaltracker.com/story/show/54480278
Assignee: nobody → dale
Blocks: 894923
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
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?
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.
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.
Whiteboard: [sprintready]
Duplicate of this bug: 894923
Summary: Enable event-target fluffing for b2g browser → Fix event-fluffing to work with ASPZ
Dale, any progress on this?
Flags: needinfo?(dale)
Summary: Fix event-fluffing to work with ASPZ → Fix event-fluffing to work with APZC
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
Flags: needinfo?(dale)
Whiteboard: [sprintready] → [sprintready][FT:Browser, KOI:P1]
Setting status to assigned for the benefit of the 1.2 dashboard
Status: NEW → ASSIGNED
Blocks: koi-browser
blocking-b2g: --- → koi+
Whiteboard: [sprintready][FT:Browser, KOI:P1] → [sprintready][UCID: Browser1, FT:Browser, KOI:P1]
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
Attachment #802449 - Flags: review?(bugmail.mozilla)
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.
Attachment #802449 - Flags: review?(bugmail.mozilla) → feedback+
Addressed feedback comments
Attachment #802449 - Attachment is obsolete: true
Attachment #802622 - Flags: review?(bugmail.mozilla)
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 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.
Attachment #802622 - Flags: review?(bugmail.mozilla) → review+
Switched UNKNOWN source to TOUCH for context menu, carrying r+
Attachment #802622 - Attachment is obsolete: true
Attachment #802814 - Flags: review+
No longer blocks: 907102
https://hg.mozilla.org/integration/mozilla-inbound/rev/42a4dec61095
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Forgot I should resolve this until it hits central
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
https://hg.mozilla.org/mozilla-central/rev/42a4dec61095
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.2 FC (16sep)
Note to QA : This won't be in today's nightly build from releng, it will be in tomorrow's build.
You need to log in before you can comment on or make changes to this bug.