Closed
Bug 775447
Opened 12 years ago
Closed 12 years ago
Let content touch-event listeners cancel async pan/zoom
Categories
(Core :: Graphics: Layers, defect)
Core
Graphics: Layers
Tracking
()
RESOLVED
FIXED
mozilla18
People
(Reporter: cjones, Assigned: drs)
References
Details
(Whiteboard: [LOE:M])
Attachments
(3 files, 4 obsolete files)
(deleted),
patch
|
drs
:
review+
drs
:
checkin+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
cjones
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
cjones
:
review+
|
Details | Diff | Splinter Review |
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.
Updated•12 years ago
|
blocking-basecamp: ? → +
Assignee | ||
Comment 1•12 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)
Reporter | ||
Comment 2•12 years ago
|
||
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•12 years ago
|
||
Attachment #646818 -
Attachment is obsolete: true
Attachment #648585 -
Flags: review?(jones.chris.g)
Reporter | ||
Comment 4•12 years ago
|
||
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•12 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 | ||
Comment 6•12 years ago
|
||
Whiteboard: [leave open]
Assignee | ||
Updated•12 years ago
|
Attachment #648771 -
Flags: checkin+
Comment 7•12 years ago
|
||
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/d8878c0bd67c - something in that push made Linux oddly unhappy.
Comment 8•12 years ago
|
||
Assignee | ||
Comment 9•12 years ago
|
||
This was backed out.
Assignee | ||
Comment 10•12 years ago
|
||
Comment 11•12 years ago
|
||
Comment 12•12 years ago
|
||
Can this be closed now, Doug? What's left?
Assignee | ||
Comment 13•12 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.
Updated•12 years ago
|
Whiteboard: [leave open] → [leave open][LOE:M]
Assignee | ||
Comment 14•12 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•12 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•12 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•12 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.
Reporter | ||
Comment 18•12 years ago
|
||
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)
Reporter | ||
Updated•12 years ago
|
Attachment #653045 -
Flags: review?(jones.chris.g) → review+
Assignee | ||
Comment 19•12 years ago
|
||
Addressed review comment.
Attachment #653044 -
Attachment is obsolete: true
Attachment #653467 -
Flags: review?(jones.chris.g)
Assignee | ||
Comment 20•12 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)
Reporter | ||
Updated•12 years ago
|
Attachment #653635 -
Flags: review?(jones.chris.g) → review+
Assignee | ||
Comment 21•12 years ago
|
||
Comment 22•12 years ago
|
||
Assignee | ||
Comment 23•12 years ago
|
||
Whoops, this should be closed.
Status: NEW → RESOLVED
Closed: 12 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.
Description
•