AsyncPanZoomController gestures are processed unreliably (every other gesture?)

RESOLVED FIXED in Firefox 21

Status

()

Core
Graphics: Layers
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: cjones, Assigned: nrc)

Tracking

({regression})

unspecified
B2G C4 (2jan on)
ARM
Gonk (Firefox OS)
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-basecamp:+, firefox19 wontfix, firefox20 wontfix, firefox21 fixed, b2g18 fixed)

Details

Attachments

(3 attachments, 1 obsolete attachment)

STR
 (1) Load nytimes.com, wait to finish
 (2) Try to fling down the page
 (3) Try to fling down the page again

You'll most likely see either (2) succeed and (3) fail, or (2) fail and (3) succeed.

It seems like the gesture detection in APZC is being messed up every other gesture.

This takes perceived browser performance to crap, and as a regression should block.

Shih-Chiang do you have cycles to take a look?
@cjones, sure, I'll take a look at this bug.
Assignee: nobody → schien
blocking-basecamp: ? → +
Created attachment 699737 [details] [diff] [review]
WIP - process all queued touch transactions after content process finishing event handler.

APZC delays gesture detection because www.nytimes.com has touch event listener. If we have more than one touch transaction in queue, APZC will only process the first touch transaction in queue. Therefore, you'll one see one gesture been triggered.
Attachment #699737 - Flags: feedback?(jones.chris.g)
(In reply to Shih-Chiang Chien [:schien] from comment #2)
> only process the first touch transaction in queue. Therefore, you'll one see
s/one see/only see
Comment on attachment 699737 [details] [diff] [review]
WIP - process all queued touch transactions after content process finishing event handler.

This seems right to me.  I reviewed the original patch but I don't remember this check specifically.

drs, do you recall why this is here?
Attachment #699737 - Flags: feedback?(jones.chris.g)
Attachment #699737 - Flags: feedback?(bugzilla)
Attachment #699737 - Flags: feedback+
Comment on attachment 699737 [details] [diff] [review]
WIP - process all queued touch transactions after content process finishing event handler.

The idea behind this code block was that we should require a preventDefault or no-preventDefault for each block of touch events from start->n*move->end/cancel. Removing this block will make us process all queued up touch events.

That means that if we touch the screen 3 independent times (i.e. I put my finger down, do some arbitrary motion, then take my finger off) before content manages to handle one of them, the first one to handle it will cause the following ones to be processed at the same time.

We're already not following the W3 spec for this, but here's the important part of what it has to say about this (http://www.w3.org/TR/touch-events/#list-of-touchevent-types):
> If the preventDefault method is called on this event, it should prevent any 
> default actions caused by any touch events associated with the same active 
> touch point, including mouse events or scrolling. 

My interpretation of this is that we should be associating each preventDefault/no-preventDefault to the series of touch events that it was handling (start->n*move->end/cancel). Removing this code would break that.

The reason this isn't working is because, if touch events with different mIdentifier fields are interspersed, only the first touchend will get sent.

I'd rather fix this another way like batching touch events into queues based on the mIdentifier field rather than chronological ordering, but that seems like a lot of work and personally I'm okay with fixing it this way for now as long as we come back to it (i.e. file tech debt bugs).
Attachment #699737 - Flags: feedback?(bugzilla) → feedback+
Created attachment 700344 [details] [diff] [review]
process all queued touch transactions after content process finishing event handler, v1

add FIXME comment.
Attachment #699737 - Attachment is obsolete: true
Attachment #700344 - Flags: review?(jones.chris.g)
Comment on attachment 700344 [details] [diff] [review]
process all queued touch transactions after content process finishing event handler, v1

This patch doesn't fix the issue for me :(.

I suspect this is related somehow to bug 828367 and async-pan-zoom detection.
Attachment #700344 - Flags: review?(jones.chris.g)
(Assignee)

Updated

6 years ago
Assignee: schien → ncameron
(In reply to Shih-Chiang Chien [:schien] from comment #6)
> Created attachment 700344 [details] [diff] [review]
> process all queued touch transactions after content process finishing event
> handler, v1
> 
> add FIXME comment.

If this patch ever sees the light of day, I'd prefer it if (a) bug(s) were filed for this instead of just adding a FIXME.

Comment 9

6 years ago
schien, do you want to keep working on this?
(In reply to Andreas Gal :gal from comment #9)
> schien, do you want to keep working on this?
I handed it over to :nrc yesterday, and I'll help him on this bug since I have bandwidth now. :)
(Assignee)

Comment 11

6 years ago
I find this difficult to recreate with flings, but pretty easy with pinch-zooms. I don't think that the issue is with prematurely clearing the queue though, we don't ever hit the path changed in the patch when we miss the gestures. Investigating...
(Assignee)

Comment 12

6 years ago
Created attachment 701112 [details] [diff] [review]
part 1: fix pinch to zoom behaviour

We're not entirely sure this is safe behaviour, but it certainly fixes the dropped pinch-to-zoom gestures.

We still need to fix the fling issue.
Attachment #701112 - Flags: review?(jones.chris.g)
Attachment #701112 - Flags: feedback?(bugzilla)
(Assignee)

Comment 13

6 years ago
Patch is joint work with schien, btw. Teamwork FTW!
Component: Graphics: Layers → Gaia
Product: Core → Boot2Gecko
Target Milestone: --- → B2G C4 (2jan on)
Attachment #701112 - Flags: review?(jones.chris.g) → review+
Component: Gaia → Graphics: Layers
Product: Boot2Gecko → Core
(Assignee)

Comment 15

6 years ago
Could someone land this to b2g18 for me please? I seem to be having hg issues with that repo :-(

Comment 17

6 years ago
Kats, have you been able to reproduce the fling issue?
(In reply to JP Rosevear [:jpr] from comment #17)
> Kats, have you been able to reproduce the fling issue?

No, I can't reproduce it either on my device (test-drivers device running the latest available build) or on a B2G Desktop gaia build.
(Assignee)

Comment 20

6 years ago
Created attachment 701844 [details] [diff] [review]
part 2: fix flings

The problem is that when the time between samples gets 'large', the velocity gets set to 0 too quickly because the way we calculate the new velocity from the friction is incorrect. I changed the way it is calculated and that fixes the bug. I adjusted the friction global to give apx the same speed of flinging.
Attachment #701844 - Flags: review?(jones.chris.g)
Attachment #701844 - Flags: feedback?(bugzilla)
Chris, if this gets an r+, could you land it as well?  Nick is in GMT, and we want to land this today.
Attachment #701844 - Flags: review?(jones.chris.g) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/66c886b7d927
https://hg.mozilla.org/releases/mozilla-b2g18/rev/f5278c18083b
Status: NEW → RESOLVED
Last Resolved: 6 years ago
status-b2g18: --- → fixed
status-firefox19: --- → wontfix
status-firefox20: --- → wontfix
status-firefox21: --- → fixed
Resolution: --- → FIXED
Whiteboard: [leave open]
backed out of inbound due to build bustage
https://hg.mozilla.org/integration/mozilla-inbound/rev/66c886b7d927
I don't have the b2g18 repo here to do the same there... please take care of that.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
the failure on Windows
e:/builds/moz2_slave/m-in-w32/build/gfx/layers/ipc/Axis.cpp(165) : error C2666: 'pow' : 6 overloads have similar conversions
ehm sorry, I posted the wrong backout changeset:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d94202f46627
https://hg.mozilla.org/integration/mozilla-inbound/rev/37eb156ec02c
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
Comment on attachment 701112 [details] [diff] [review]
part 1: fix pinch to zoom behaviour

Review of attachment 701112 [details] [diff] [review]:
-----------------------------------------------------------------

Clearing from my review queue, also f+ FWIW at this point.
Attachment #701112 - Flags: feedback?(bugzilla) → feedback+
Comment on attachment 701844 [details] [diff] [review]
part 2: fix flings

Review of attachment 701844 [details] [diff] [review]:
-----------------------------------------------------------------

Clearing from my review queue, also f+ FWIW at this point.
Attachment #701844 - Flags: feedback?(bugzilla) → feedback+
You need to log in before you can comment on or make changes to this bug.