Closed Bug 775447 Opened 8 years ago Closed 8 years ago

Let content touch-event listeners cancel async pan/zoom

Categories

(Core :: Graphics: Layers, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla18
blocking-kilimanjaro ?
blocking-basecamp +

People

(Reporter: cjones, Assigned: drs)

References

Details

(Whiteboard: [LOE:M])

Attachments

(3 files, 4 obsolete files)

The current async pan/zoom code just ignores them and always uses its heuristics.  We instead want to check if content has register touch-event listeners, and give them a chance to preventDefault() the pan/zoom animations before starting them.

We have all the platform support for this that we need.  This bug covers gluing that into the new pan/zoom system.

I think this should block basecamp because it will be a fairly important web-compat point, and isn't very risky.
blocking-basecamp: ? → +
Enable us to watch for adding and removing touch listeners. The removal code doesn't do anything because we don't get a "dom-touch-listener-removed" event, or anything like that.
Assignee: nobody → bugzilla
Attachment #646818 - Flags: review?(jones.chris.g)
Comment on attachment 646818 [details] [diff] [review]
Properly track number of DOM touch listeners in AsyncPanZoomController

The current mechanism for tracking this is not as precise as you're using it here.  We need to keep a bool "may have touch listeners", and then only reset that on first paint.
Attachment #646818 - Flags: review?(jones.chris.g)
Attachment #646818 - Attachment is obsolete: true
Attachment #648585 - Flags: review?(jones.chris.g)
Comment on attachment 648585 [details] [diff] [review]
Properly track number of DOM touch listeners in AsyncPanZoomController

>diff --git a/gfx/layers/ipc/AsyncPanZoomController.cpp b/gfx/layers/ipc/AsyncPanZoomController.cpp

>+     mHasTouchListeners(false)

mMayHaveTouchListener.  We don't know for sure here.

>diff --git a/gfx/layers/ipc/AsyncPanZoomController.h b/gfx/layers/ipc/AsyncPanZoomController.h
>--- a/gfx/layers/ipc/AsyncPanZoomController.h
>+++ b/gfx/layers/ipc/AsyncPanZoomController.h
>@@ -93,16 +93,26 @@ public:
>    * using CSS pixels everywhere inside here, but in this case we need to know
>    * how large of a displayport to set so we use these dimensions plus some
>    * extra.
>    *
>    * XXX: Use nsIntRect instead.
>    */
>   void UpdateViewportSize(int aWidth, int aHeight);
> 
>+  /**
>+   * A DOM touch listener has been added. When called, we enable the machinery
>+   * that allows touch listeners to preventDefault any touch inputs. This should
>+   * not be called unless there are actually touch listeners as it introduces a

  it /can/ introduce unbounded lag because in general it requires a
  round-trip through the content main thread.

>+   * great amount of lag in response time. No locking is done here since we're
>+   * generally pretty lazy about actually allowing DOM to hook and prevent
>+   * touches to begin with.

I don't understand this part of the comment.

No need to mention locking here, but you do want to mention which
thread it's legal to call this method on.  I believe it should be
UI-thread only.

>+  // Stores whether or not there are touch listeners in the DOM for the frame
>+  // we're managing. If non-zero, we must allow them to intercept touches which
>+  // adds some additional latency to each touch.

Stale comment.  Be sure to note that this is a conservative
approximation of whether we /might/ have touch listeners.

r=me with those changes
Attachment #648585 - Flags: review?(jones.chris.g) → review+
Review comments fixed, r+ carried.

Also added a single new line to AsyncPanZoomController::NotifyLayersUpdated() to reset the mMayHaveTouchListeners flag to false on first paint.
Attachment #648585 - Attachment is obsolete: true
Attachment #648771 - Flags: review+
Attachment #648771 - Flags: checkin+
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/d8878c0bd67c - something in that push made Linux oddly unhappy.
This was backed out.
Can this be closed now, Doug? What's left?
This doesn't actually do anything yet, it tracks whether or not there are listeners. I'm working on actually doing stuff with that information.
Whiteboard: [leave open] → [leave open][LOE:M]
This allows content to actually preventDefault touch events. Unfortunately we ended up not using the code I previously pushed in this bug.
Attachment #653044 - Flags: review?(jones.chris.g)
This removes most of the code from attachment 648771 [details] [diff] [review]. It's not a direct backout since removing some of the code I added would break other stuff that has been added which depends on it now. For the purposes of review, I don't know that this needs a very serious look through.
Attachment #653045 - Flags: review?(jones.chris.g)
Try push:
https://tbpl.mozilla.org/?tree=Try&rev=fb2b71f60006

(these patches are both in there; it got coalesced after I messed up and pushed only one of them to try)
Retrying that push here:
https://tbpl.mozilla.org/?tree=Try&rev=c395efd4bb80

There was an intermittent on inbound/central matching the one I keep getting for Win opt M1, so I rebased it and pushed again.
Comment on attachment 653044 [details] [diff] [review]
Let touch-event listeners cancel async pan/zoom

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

> bool
> TabChild::RecvRealTouchEvent(const nsTouchEvent& aEvent)
> {
>     nsTouchEvent localEvent(aEvent);
>     nsEventStatus status = DispatchWidgetEvent(localEvent);
>+    SendContentReceivedTouch(nsIPresShell::gPreventMouseEvents);

Let's do this only if mayHaveContentTouchListeners.

Otherwise looks fine.  Would like to see the next version.
Attachment #653044 - Flags: review?(jones.chris.g)
Attachment #653045 - Flags: review?(jones.chris.g) → review+
Addressed review comment.
Attachment #653044 - Attachment is obsolete: true
Attachment #653467 - Flags: review?(jones.chris.g)
Forgot to qref. Much better method of getting the inner window.
Attachment #653467 - Attachment is obsolete: true
Attachment #653467 - Flags: review?(jones.chris.g)
Attachment #653635 - Flags: review?(jones.chris.g)
Attachment #653635 - Flags: review?(jones.chris.g) → review+
Whoops, this should be closed.
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [leave open][LOE:M] → [LOE:M]
Target Milestone: --- → mozilla18
You need to log in before you can comment on or make changes to this bug.