Let content touch-event listeners cancel async pan/zoom

RESOLVED FIXED in mozilla18

Status

()

defect
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: cjones, Assigned: drs)

Tracking

Trunk
mozilla18
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-kilimanjaro:?, blocking-basecamp:+)

Details

(Whiteboard: [LOE:M])

Attachments

(3 attachments, 4 obsolete attachments)

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: ? → +
Assignee

Comment 1

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

Comment 3

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

Comment 5

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

Updated

7 years ago
Attachment #648771 - Flags: checkin+
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/d8878c0bd67c - something in that push made Linux oddly unhappy.
Assignee

Comment 9

7 years ago
This was backed out.
Can this be closed now, Doug? What's left?
Assignee

Comment 13

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

Comment 14

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

Comment 15

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

Comment 16

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

Comment 17

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

Comment 19

7 years ago
Addressed review comment.
Attachment #653044 - Attachment is obsolete: true
Attachment #653467 - Flags: review?(jones.chris.g)
Assignee

Comment 20

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

Comment 23

7 years ago
Whoops, this should be closed.
Status: NEW → RESOLVED
Closed: 7 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.