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

RESOLVED FIXED in mozilla18

Status

()

defect
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: drs, Assigned: drs)

Tracking

Trunk
mozilla18
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

Assignee

Description

7 years ago
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.
Assignee

Comment 1

7 years ago
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)
Assignee

Comment 4

7 years ago
(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));
Assignee

Updated

7 years ago
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-
Assignee

Comment 8

7 years ago
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+
Assignee

Comment 10

7 years ago
(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+
Assignee

Comment 11

7 years ago
I don't have my LDAP for now, this needs a checkin.
Whiteboard: [checkin-needed]
Assignee

Comment 12

7 years ago
r+ carried, fixed mistake in rebase.
Attachment #661365 - Flags: review+
Assignee

Updated

7 years ago
Keywords: checkin-needed
Whiteboard: [checkin-needed]
Attachment #661237 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/6875465535eb
Status: NEW → RESOLVED
Closed: 7 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.