Closed Bug 787549 Opened 8 years ago Closed 8 years ago

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

Categories

(Core :: Graphics: Layers, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: drs, Assigned: drs)

References

Details

Attachments

(1 file, 3 obsolete files)

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.
Proposed patch.
Assignee: nobody → bugzilla
Attachment #657430 - Flags: review?(jones.chris.g)
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.
Attachment #657430 - Flags: review?(jones.chris.g)
(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.
When apzc is enabled, we only want to send touch events if there may be a content listener.
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));
Attachment #657430 - Flags: review?(jones.chris.g)
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.
Attachment #657430 - Flags: review?(jones.chris.g) → review-
Fixed bugs.
Attachment #657430 - Attachment is obsolete: true
Attachment #659305 - Flags: review?(jones.chris.g)
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.
Attachment #659305 - Flags: review?(jones.chris.g) → review+
(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.
Attachment #659305 - Attachment is obsolete: true
Attachment #661237 - Flags: review+
I don't have my LDAP for now, this needs a checkin.
Whiteboard: [checkin-needed]
r+ carried, fixed mistake in rebase.
Attachment #661365 - Flags: review+
Keywords: checkin-needed
Whiteboard: [checkin-needed]
Attachment #661237 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/6875465535eb
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Duplicate of this bug: 782321
You need to log in before you can comment on or make changes to this bug.