Give content process time to capture touch events and prevent panning

VERIFIED FIXED

Status

VERIFIED FIXED
8 years ago
8 years ago

People

(Reporter: stechz, Assigned: wesj)

Tracking

Dependency tree / graph

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

8 years ago
In bug 544614 we now allow the content process to capture drags, but we allow panning until that happens. We should give the content process a few milliseconds before we start panning.
(Reporter)

Updated

8 years ago
Depends on: 544614
I'm a bit worried that it might take arbitrarily long for the content process to respond to the preventDefault method, so the timeout would either be very large (and interfere with panning responsiveness) or would sometimes too small (and we would accidentally pan the UI too often).  For example, Google Maps seems to often be busy processing XHR requests and loading images, which slows down its event processing.

If we add a flag to tell whether a page has touch event listeners, then we can at least avoid any impact on pages with no listeners.  Felipe says we should file a separate bug if we decide we need this flag.
(Assignee)

Comment 2

8 years ago
Created attachment 528198 [details] [diff] [review]
Patch

Cancels panning by default on pages with touch listeners. Re-enables it after a 300 ms timeout if the page doesn't respond, but pages should likely always respond since I'm still firing touch events whether they have touch listeners or not.

This makes it very difficult, if not impossible to pan in the sidebars on Google Maps.
Assignee: nobody → wjohnston
Attachment #528198 - Flags: review?
(Assignee)

Updated

8 years ago
Attachment #528198 - Flags: review? → review?(mbrubeck)
(Assignee)

Comment 3

8 years ago
Note this has to apply on top of the patches in 544614 648753, and 651984.
Depends on: 651984
Comment on attachment 528198 [details] [diff] [review]
Patch

>+++ b/mobile/chrome/content/bindings/browser.xml
>+      <field name="hasTouchListeners">false</field>

It looks like hasTouchListeners is updated only during progress and pageshow notifications.  Will this miss touch listeners that are added dynamically, after loading is complete?  If so, is there a good way to fix that?  If we can't do it easily, we can leave this for a followup.

>+++ b/mobile/chrome/content/browser.js

>+    Elements.browsers.customDragger.allowContentPanning = !browser.hasTouchListeners;
>+    if (browser.hasTouchListeners)
>+      this.touchTimeout = setTimeout(function() {
>+        Elements.browsers.customDragger.allowContentPanning = false;
>+      }, kTouchTimeout)

Shouldn't we set "allowContentPanning = true" in the setTimeout callback?


>+++ b/mobile/chrome/content/content.js

>+    sendAsyncMessage("Content:EventCanceled", { messageId: json.messageId,
>+                                                disableChromePanning: cancelled });

Nit: Can we change the name from "Content:EventCanceled" to something like "Content:EventComplete"?

r=mbrubeck with the above questions answered.
Attachment #528198 - Flags: review?(mbrubeck) → review+
Comment on attachment 528198 [details] [diff] [review]
Patch

I don't like this patch because I think hasTouchListeners should not be on the browser binding. Sounds like a method that should be a message. Matt even points out the staleness issue.

Also, for messages that live in browser.js/content.js we use the "Browser:" prefix. Messages that live in the browser binding use the "Content:" prefix. I'm looking at "Content:EventCanceled" - also, shouldn't that be "Content:EventCancelled" ?
(Assignee)

Comment 6

8 years ago
Created attachment 528381 [details] [diff] [review]
Patch v2

> It looks like hasTouchListeners is updated only during progress and pageshow

Yeah, not sure what the best solution here is. I refactored here so that we can ask about touchHandlers through an asyncMessage/return. I call that after FirstPaint so it should update for every page sometime close to page load...

I am also disabling any Browser:EventComplete return unless the page has touch handlers, so I can assume that if I ever receive an Browser:EventComplete message once, I should delay panning from there on out for this page.

The value is being stored in the ContentTouchHandler now.
Attachment #528198 - Attachment is obsolete: true
Attachment #528381 - Flags: review?(mark.finkle)
Some issues I missed in my previous review:

1) We always send disableChromePanning:false in response to Browser:MouseUp.  This can allow kinetic panning to happen even after panning was prevented.  You can reproduce this by swiping within the toolbar on Google Maps, or by swiping while starting in the blue DIV on http://people.mozilla.com/~mbrubeck/test/touch-pan.html

Probably we should suppress kinetic panning completely if the page consumes (i.e. preventDefaults) the touch events.

2) We should always check for an existing timeout before starting a new one.  In the (rare) case where we tapDown is called a second time before the first EventCanceled response is received, we will currently end up with two timeouts and one of them will never be canceled.
Status: NEW → ASSIGNED
(Assignee)

Comment 8

8 years ago
Created attachment 528616 [details] [diff] [review]
Patch v3

Addresses Matt's comments. I am just not sending cancel events to the parent from TouchEnd/MouseUp. It might be nice if we had methods/events to kill kinetic panning without also killing the entire input handler, but this seems much simpler for now.
Attachment #528381 - Attachment is obsolete: true
Attachment #528381 - Flags: review?(mark.finkle)
Attachment #528616 - Flags: review?(mark.finkle)
Comment on attachment 528616 [details] [diff] [review]
Patch v3

Overall, I think some terminology is a bit confusing, especially coming into this code late. To me, the content can try to capture the input. We use "Browser:MouseXxx" messages, so using "Browser", "Mouse" and "Capture" in some way seems to make sense to me. Also, we should always frame the naming convention in the same logic, so lets always talk about whether content has captured the mouse, and not about whether chrome panning is disabled.

>diff --git a/mobile/chrome/content/bindings/browser.js b/mobile/chrome/content/bindings/browser.js

> let ContentScroll =  {
>   _scrollOffset: { x: 0, y: 0 },
> 
>   init: function() {
>     addMessageListener("Content:SetCacheViewport", this);
>     addMessageListener("Content:SetWindowSize", this);
>+    addMessageListener("Content:UpdateTouchListeners", this);

I don't think this belongs in the binding. This seems like an application feature and should be in content.js

>+      case "Content:UpdateTouchListeners": {
>+        json = {
>+          hasTouchListeners: content.QueryInterface(Ci.nsIInterfaceRequestor)
>+                                    .getInterface(Ci.nsIDOMWindowUtils)
>+                                    .mayHaveTouchEventListeners
>+        }
>+        sendAsyncMessage("Content:UpdateTouchListeners", json);

We should stay away from naming incoming and outgoing messages the same name. We typically use "Content:UpdateTouchListeners:Return"
Moving to content.js (app specific) and using the above "capture" logic would make it "Browser:MouseCapture" and "Browser:MouseCapture:Return"

>diff --git a/mobile/chrome/content/browser.js b/mobile/chrome/content/browser.js

> const kDefaultBrowserWidth = 800;
> const kFallbackBrowserWidth = 980;
>+// allow panning after this timeout on pages with registered touch listeners
>+const kTouchTimeout = 300;

blank line to separate

> Browser.WebProgress = function WebProgress() {

>+  messageManager.addMessageListener("Content:UpdateTouchListeners", this);

> Browser.WebProgress.prototype = {
>   receiveMessage: function receiveMessage(aMessage) {

>+      case "Content:UpdateTouchListeners": {
>+        ContentTouchHandler.pageHasTouchListeners = json.hasTouchListeners;
>+        break;
>+      }

Move this all to ContentTouchHandler

>       browser.messageManager.addMessageListener("MozScrolledAreaChanged", aTab.scrolledAreaChanged);
>+      browser.messageManager.sendAsyncMessage("Content:UpdateTouchListeners", {});

This can stay I guess. I don't like the weakness of the approach, but it's better than nothing.

> const ContentTouchHandler = {

>+  pageHasTouchListeners: false,

I like the "capture" terminology here again: "contentMouseCapture"

>       case "Browser:EventComplete":
>-        Elements.browsers.customDragger.allowContentPanning = false;
>+        this.pageHasTouchListeners = true;
>+        if (this.touchTimeout)
>+          clearTimeout(this.touchTimeout);
>+        Elements.browsers.customDragger.allowContentPanning = !aMessage.json.disableChromePanning;

* You need to set this.touchTimeout to null when clearing it, since you test for it in the if condition
* allowContentPanning -> contentMouseCapture
* disableChromePanning -> contentMouseCapture

>-    Elements.browsers.customDragger.allowContentPanning = true;
>+
>+    Elements.browsers.customDragger.allowContentPanning = !this.pageHasTouchListeners;

contentMouseCapture

>+
>+    if (this.touchTimeout)
>+      clearTimeout(this.touchTimeout);

set this.touchTimeout to null

>diff --git a/mobile/chrome/content/content.js b/mobile/chrome/content/content.js

>   receiveMessage: function(aMessage) {
>-    if (Util.isParentProcess())
>+    if (!content.QueryInterface(Ci.nsIInterfaceRequestor)
>+                .getInterface(Ci.nsIDOMWindowUtils)
>+                .mayHaveTouchEventListeners)

we really don't care about inParentProcess anymore?

>       return;
> 
>+
>     let json = aMessage.json;

extra blank separate is not needed

>     let cancelled = false;

you spell cancelled and canceled. cancelled seems the more proper way

>+    if (aMessage.name != "Browser:MouseUp")
>+      sendAsyncMessage("Browser:EventComplete", { messageId: json.messageId,
>+                                                  disableChromePanning: cancelled });

contentMouseCapture
Attachment #528616 - Flags: review?(mark.finkle) → review-
(Assignee)

Comment 10

8 years ago
Created attachment 528952 [details] [diff] [review]
Patch v4

Hopefully this is the last round here:

Moved all of the capturing code to browser.js and content.js. I am not attempting to cache this anymore at all, which, I realized was causing bugs when hopping between tabs. I'm now updating the behavior every time a tab is set to active, and after Browser:FirstPaint. Also had to add a messageId to this to make it happy.

I agree, this feels like it is going to be work to maintain. I'm not sure what the right solution is. I'm on the lookout for a better message that fires after the page's initial onload handler, and every time the user switches tabs? Also, we could cache this on the browser element or with the Tab-object to save ourselves a little work.

Renamed "Content:UpdateTouchListeners" to "Content:CanCaptureMouse" and "Content:CanCaptureMouse:Return".

The rest of the changes follow the review comments ( I hope :) ).
Attachment #528616 - Attachment is obsolete: true
(In reply to comment #10)
> I agree, this feels like it is going to be work to maintain. I'm not sure what
> the right solution is. I'm on the lookout for a better message that fires after
> the page's initial onload handler, and every time the user switches tabs? Also,
> we could cache this on the browser element or with the Tab-object to save
> ourselves a little work.

Yes, it seems like "contentCanCaptureMouse" should be a property of the tab object, rather than the global ContentTouchHandler object.  Then you wouldn't need to update it asynchronously after switching tabs.

I think the ideal case for us (perhaps as a longer-term followup) would be a message that is sent when "mayHaveTouchEventListeners" changes.  I don't know whether it is possible to implement this reasonably and efficiently.  (Adding Olli in case he has any comments.)
(In reply to comment #11)
> I think the ideal case for us (perhaps as a longer-term followup) would be a
> message that is sent when "mayHaveTouchEventListeners" changes.  I don't know
> whether it is possible to implement this reasonably and efficiently.  (Adding
> Olli in case he has any comments.)

It should be easy to implement it, and should be quite efficient too.
I'd assume window could fire "MozMayHaveTouchEvents" or some such to
chrome (TabChildGlobal) when it gets the first touch event listener.
Btw, what is this bug about? HTML drag events[1]
or events used for panning?

[1] http://www.whatwg.org/specs/web-apps/current-work/multipage/dnd.html#dnd
pushed:
http://hg.mozilla.org/mozilla-central/rev/7d40685986cc

we can file followup bugs for the Tab object refactor.

Olli - this bug getting touch events and chrome panning to play nicely together.
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Summary: Give content process time to capture drag events → Give content process time to capture touch events and prevent panning
(In reply to comment #12)
> (In reply to comment #11)
> > I think the ideal case for us (perhaps as a longer-term followup) would be a
> > message that is sent when "mayHaveTouchEventListeners" changes.  I don't know
> > whether it is possible to implement this reasonably and efficiently.  (Adding
> > Olli in case he has any comments.)
> 
> It should be easy to implement it, and should be quite efficient too.
> I'd assume window could fire "MozMayHaveTouchEvents" or some such to
> chrome (TabChildGlobal) when it gets the first touch event listener.

I filed bug 654129 for this.

Comment 16

8 years ago
How can I verify this?
(In reply to comment #16)
> How can I verify this?

On this test case, touching the "prevent move" box and quickly dragging should not pan the page:

http://limpet.net/w3/touchevents/preventDefault.html

Comment 18

8 years ago
VERIFIED FIXED on:

Mozilla /5.0 (Android;Linux armv7l;rv:7.0a1) Gecko/20110602 Firefox/7.0a2 Fennec/7.0a1 

Mozilla /5.0 (Android;Linux armv7l;rv:6.0a2) Gecko/20110601 Firefox/6.0a2 Fennec/6.0a2

Device: HTC Desire Z (Android 2.2)
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.