787549 - B2G: Stop simulating mouse events unless there's a tap

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Doug Sherk (:drs) (inactive)]

We're unfortunately following links when we shouldn't because we're simulating mouse events when we really shouldn't be. This mouse synthesis code is a relic of the days when we couldn't detect taps. We need to hook up the AsyncPanZoomController tapping code to mouse events.

Comment 1

[Doug Sherk (:drs) (inactive)]

Attachment: Stop simulating mouse events unless there's a tap

Proposed patch.

Comment 2

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Nit: "relict".

Comment 3

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 657430 [details] [diff] [review]
Stop simulating mouse events unless there's a tap


>diff --git a/dom/ipc/PBrowser.ipdl b/dom/ipc/PBrowser.ipdl

>     /**
>+     * Requests handling of a single tap. |point| is in CSS pixels, relative to
>+     * the scroll offset. This message is expected to send a "mousemove",
>+     * "mousedown" and "mouseup" series of events at this point.

It's in widget space, actually (or should be).  You're dispatching it
through the PuppetWidget, remember.

>diff --git a/dom/ipc/TabChild.cpp b/dom/ipc/TabChild.cpp

> TabChild::RecvRealTouchEvent(const nsTouchEvent& aEvent)
> {

>-      SendContentReceivedTouch(nsIPresShell::gPreventMouseEvents);
>+        SendContentReceivedTouch(nsIPresShell::gPreventMouseEvents);

There's not much point in doing this restructuring since we shouldn't
reach this code.  I would just revert this part of the patch.

I don't see the code that prevents TabParent from sending touch events
when it's focused.  Where is that?  We should allow RenderFrameParent 
to veto event forwarding when we're detecting taps.

Comment 4

[Doug Sherk (:drs) (inactive)]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #3)
> Comment on attachment 657430 [details] [diff] [review]
> Stop simulating mouse events unless there's a tap
> 
> 
> >diff --git a/dom/ipc/PBrowser.ipdl b/dom/ipc/PBrowser.ipdl
> 
> >     /**
> >+     * Requests handling of a single tap. |point| is in CSS pixels, relative to
> >+     * the scroll offset. This message is expected to send a "mousemove",
> >+     * "mousedown" and "mouseup" series of events at this point.
> 
> It's in widget space, actually (or should be).  You're dispatching it
> through the PuppetWidget, remember.

Yeah, you're right. I c&p'd that comment from the HandleDoubleTap function, and I think I wrote that comment before I really understood the coordinate spaces.

> >diff --git a/dom/ipc/TabChild.cpp b/dom/ipc/TabChild.cpp
> 
> > TabChild::RecvRealTouchEvent(const nsTouchEvent& aEvent)
> > {
> 
> >-      SendContentReceivedTouch(nsIPresShell::gPreventMouseEvents);
> >+        SendContentReceivedTouch(nsIPresShell::gPreventMouseEvents);
> 
> There's not much point in doing this restructuring since we shouldn't
> reach this code.  I would just revert this part of the patch.
> 
> I don't see the code that prevents TabParent from sending touch events
> when it's focused.  Where is that?  We should allow RenderFrameParent 
> to veto event forwarding when we're detecting taps.

Hmm, I thought I understood this from our discussion that we want to always send touch events, just only block synthesized mouse events when we want to control tapping from APZC.

Another thing of note that I found out is that touch events seem to be what highlight links when you touch them, but they don't make us actually click them.

Comment 5

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

When apzc is enabled, we only want to send touch events if there may be a content listener.

Comment 6

[Cervantes Yu [:cyu] [:cervantes]]

From the patch, there seems to be a bug in HandleSingleTap():

+  virtual void HandleSingleTap(const nsIntPoint& aPoint) MOZ_OVERRIDE
                ^^^^^^^^^^^^^^^
+  {
+    if (MessageLoop::current() != mUILoop) {
+      // We have to send this message from the "UI thread" (main
+      // thread).
+      mUILoop->PostTask(
+        FROM_HERE,
+        NewRunnableMethod(this, &RemoteContentController::HandleDoubleTap,
                                                           ^^^^^^^^^^^^^^^
+                          aPoint));

Comment 7

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 657430 [details] [diff] [review]
Stop simulating mouse events unless there's a tap

If this breaks mouse events to remote content, you won't even be able to use the homescreen anymore.

Comment 8

[Doug Sherk (:drs) (inactive)]

Attachment: Stop simulating mouse events unless there's a tap

Fixed bugs.

Comment 9

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 659305 [details] [diff] [review]
Stop simulating mouse events unless there's a tap

>diff --git a/dom/ipc/TabChild.cpp b/dom/ipc/TabChild.cpp

> bool
>+TabChild::RecvHandleSingleTap(const nsIntPoint& aPoint)
>+{

>+  RecvMouseEvent(NS_LITERAL_STRING("mousedown"), aPoint.x, aPoint.y, 0, 1, 0, false);
>+  RecvMouseEvent(NS_LITERAL_STRING("mouseup"), aPoint.x, aPoint.y, 0, 1, 0, false);
>+

Isn't the contract here that we have to send a mousemove before
mouseup?

>diff --git a/dom/ipc/TabChild.h b/dom/ipc/TabChild.h

>+    // Sends a simulated mouse event from a touch event for compatibility.
>+    void SendSynthesizedMouseEvent(const nsTouchEvent& aEvent);
>+

"Send" here is confusing in context with IPC code; let's call this
"DispatchSynthesizedMouseEvent()" instead.

r=me with question answered and name change.

Comment 10

[Doug Sherk (:drs) (inactive)]

Attachment: Stop simulating mouse events unless there's a tap

(In reply to Chris Jones [:cjones] [:warhammer] from comment #9)
> Comment on attachment 659305 [details] [diff] [review]
> Stop simulating mouse events unless there's a tap
> 
> >diff --git a/dom/ipc/TabChild.cpp b/dom/ipc/TabChild.cpp
> 
> > bool
> >+TabChild::RecvHandleSingleTap(const nsIntPoint& aPoint)
> >+{
> 
> >+  RecvMouseEvent(NS_LITERAL_STRING("mousedown"), aPoint.x, aPoint.y, 0, 1, 0, false);
> >+  RecvMouseEvent(NS_LITERAL_STRING("mouseup"), aPoint.x, aPoint.y, 0, 1, 0, false);
> >+
> 
> Isn't the contract here that we have to send a mousemove before
> mouseup?

No, I talked with wesj and he said that mousemove is unnecessary. My testing seems to indicate this is true too.

r+ carried.

Comment 11

[Doug Sherk (:drs) (inactive)]

I don't have my LDAP for now, this needs a checkin.

Comment 12

[Doug Sherk (:drs) (inactive)]

Attachment: Stop simulating mouse events unless there's a tap

r+ carried, fixed mistake in rebase.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6875465535eb

Should this have a test?

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6875465535eb